Skip to content

fix(core): preserve every reasoning episode's signature during history consolidation - #8260

Open
netbrah wants to merge 20 commits into
QwenLM:mainfrom
netbrah:fix/geminichat-thought-consolidation
Open

fix(core): preserve every reasoning episode's signature during history consolidation#8260
netbrah wants to merge 20 commits into
QwenLM:mainfrom
netbrah:fix/geminichat-thought-consolidation

Conversation

@netbrah

@netbrah netbrah commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

What this PR does

geminiChat.ts's turn-consolidation step merged every thought-flagged part in a model turn into a single blob and kept only the first thoughtSignature it saw. A turn with more than one distinct reasoning episode — one reasoning span per parallel tool call, on both Anthropic interleaved thinking and OpenAI Responses reasoning items — silently lost every signature after the first, and hoisted the merged blob to the front of the turn regardless of where the episodes actually occurred relative to the tool calls.

This replaces the merge-all/keep-first-signature pass with a single-pass algorithm that:

  1. Closes ("flushes") the current reasoning episode when a non-thought part appears, or when a thought part carries fresh text while the open episode already has both accumulated text and a signature — the boundary between two genuinely distinct episodes.
  2. Concatenates (not "keeps only the first") text and signature fragments within an open episode, so a signature split across multiple stream chunks is reassembled correctly instead of truncated.

Each episode is now preserved as its own Part in its original position, matching how other TypeScript/Rust agent harnesses represent multi-episode reasoning (see Evidence below).

Two related bugs surfaced while implementing this and are fixed in the same PR, since both would otherwise undo the primary fix in specific cases:

  • The XML-tool-call-recovery fallback (Model outputs XML-style tool calls as plain text instead of structured function calls in long sessions #8003) identified "text parts to consume and replace with remainingText" via a bare .text !== undefined check. Since a reasoning episode Part always has a .text field (even '' for a signature-only episode), this check also matched reasoning episodes — silently deleting the turn's reasoning text and signature whenever XML recovery fired on a turn that also carried one. Fixed to use the existing isValidNonThoughtTextPart predicate.
  • mergeConsecutiveAssistantMessages in anthropicContentGenerator/converter.ts unconditionally hoisted all thinking blocks to the front when merging two adjacent assistant messages, which would silently undo this fix's chronological ordering the moment two assistant messages needed merging. Since interleaved-thinking-2025-05-14 is unconditionally enabled whenever thinking is set, thinking blocks no longer need to lead; changed to straight concatenation.

Fixes #8258.

Anthropic converter changes (same PR, previously undescribed)

Roughly half this diff lives in anthropicContentGenerator/. It is not incidental — the geminiChat.ts fix is what makes these shapes reachable, so they ship together:

  • mergeConsecutiveAssistantMessages: hoist-all-thinking to straight concatenation. Covered above.

  • New ensureLeadingThinkingOnToolUseAssistantMessages pass (gated on the outgoing request's real thinking.type === 'enabled'). Because reasoning is no longer hoisted to parts[0], an ordinary "say a line, then think, then call a tool" turn now converts to [text, thinking, tool_use], which manual-mode extended thinking rejects. This pass moves only the first contiguous thinking run to the front, so the interleaving this PR is about is preserved.

    It applies to every assistant message carrying a tool_use, not just the latest one. Scoping it to the latest message (an earlier revision of this PR) was wrong twice over: An error occurred when calling the DeepSeek v4 Pro model. #3786 describes the rejection against a prior assistant turn, so a turn repaired while it was current reverts to the bad shape on the next request and fails one turn later; and making a turn's serialization depend on its position in history means the same turn goes out two different ways on consecutive requests, which — since addCacheControlToMessages anchors on the last user message — rewrites the cached prefix and forces a full prompt-cache re-read every turn. Pinned by the serializes a turn identically whether or not a later turn follows it test.

  • stripTrailingAssistantPrefill moved earlier in the pipeline, ahead of dropEmptyTextThinkingBlocks / the second stripAssistantThinking / mergeConsecutiveUserMessages, so that dropEmptyTextThinkingBlocks's one-shot "which message is latest" computation reflects the array's true final shape. The "history ends with a user message" invariant survives the reorder (dropEmptyTextThinkingBlocks skips latestAssistantIdx; mergeConsecutiveUserMessages cannot change the terminal role).

  • New dropDanglingUnsignedTrailingThought helper, applied at three call sites: end of per-stream consolidation, inside XML tool-call recovery immediately before the recovered functionCall parts are appended, and on a truncated turn's own parts before coalesceRecoveryPairs. An unsigned trailing episode is a reasoning span whose terminating signature never arrived; pairing it with a tool_use permanently wedges the session once the tool result returns.

Evidence / prior art

Checked how other agent harnesses represent multi-episode reasoning in a single turn — none merge separate episodes into one object:

  • OpenAI Codex (codex-rs/protocol/src/models.rs:314-324) — ResponseItem::Reasoning is a distinct history item per reasoning episode.
  • OpenCode (anomalyco/opencode, packages/opencode/src/session/message-v2.ts) — each reasoning span is its own { type: "reasoning", text, metadata } part; the file has an explicit comment describing this exact multi-episode-per-turn shape for Anthropic adaptive thinking.
  • Vercel AI SDK (vercel/ai) — LanguageModelV2Reasoning is one variant in a LanguageModelV2Content[] array; multiple reasoning spans are multiple array entries.

Reviewer Test Plan

How to verify

cd packages/core && npx vitest run src/core/geminiChat.test.ts src/core/anthropicContentGenerator/converter.test.ts

Expected: all tests pass (369 across both files). Key scenarios covered:

  • A turn with reasoning episodes interleaved with parallel tool calls preserves each episode's own signature.
  • Back-to-back reasoning episodes with no intervening tool call still split correctly once the first has a signature.
  • A signature arriving split across multiple stream chunks concatenates correctly instead of truncating.
  • A reasoning episode co-occurring with an XML-tool-call-recovery turn (Model outputs XML-style tool calls as plain text instead of structured function calls in long sessions #8003) survives in history with its text and signature intact.
  • mergeConsecutiveAssistantMessages preserves chronological order across a merge instead of hoisting thinking blocks to the front.

Known, documented residual limitations (not fixed here)

All three are called out in code comments and pinned by tests that assert the current behavior, so revisiting any of them turns a test red on purpose.

  1. Two unsigned back-to-back thought parts merge into one episode. The boundary heuristic has no signal to split on without a signature to test. Consistent with both wires' invariant that every episode ends in a signature-only chunk; not reachable via Anthropic interleaved thinking or OpenAI Responses reasoning items as implemented.
  2. Two adjacent text-less signed thought parts concatenate their signatures into one {text:'', thought:true, thoughtSignature:'AB'} part that is valid for neither block. This is the mirror image of (1) and is not disambiguable at this layer — intra-episode signature fragmentation is exactly what the concatenation exists to serve. Prior behavior dropped the signature entirely in this shape, so it trades a lossy result for a corrupt-on-replay one (a bad signature 400s where a missing one merely degrades). Unreachable on the Anthropic wire, where thinking blocks always carry text; reachable on the OpenAI Responses wire when reasoning summaries are disabled and only encrypted_content is returned, so flagging it explicitly for feat(core): add OpenAI Responses API content generator #8169 rather than letting that PR inherit it silently.
  3. dropDanglingUnsignedTrailingThought has an accepted false positive. A non-signing provider (DeepSeek) truncated mid-reasoning after a tool call produces the same array shape as a truncated signing-provider episode, and its trailing reasoning is dropped from both history and the JSONL record. Gating the pop on "this turn carries at least one signature" fixes this call site but is wrong at the recovery-coalescing site, where a truncated turn legitimately has no signature anywhere yet. Losing a trailing reasoning fragment for a provider that never validates signatures is the cheaper failure than permanently wedging a session that does. Separately, the coalescing call site mutates in-memory history only — recordAssistantTurn has already written the turn to disk, so --resume can rehydrate the dangling episode. That is inherited drift in the recovery-coalescing mechanism as a whole (the dropped recovery pair is likewise already on disk); closing it belongs at the persistence layer.

Risk & Scope

  • Touches shared history-consolidation code used by every wire (Gemini, Anthropic, OpenAI, OpenAI Responses), so the change is scoped tightly to the thought-part consolidation loop and the two related call sites it affects; no unrelated refactors.
  • Independent of feat(core): add OpenAI Responses API content generator #8169 (OpenAI Responses API) — this PR does not depend on or modify any file introduced by that PR.
  • Breaking changes / migration notes: one deliberate, previously-undeclared change. recordAssistantTurn now records every consolidated part verbatim, where the old code rebuilt the record as [thought?, {text: contentText}?, ...functionCalls]. Because redactStructuredOutputArgsForRecording returns null for every non-functionCall part, media parts were previously never recorded; inlineData/fileData now land in the session JSONL. That is the right call for --resume fidelity, but it does mean model-produced base64 media persists on disk, so it is called out explicitly rather than shipped as a silent side effect.
  • Single-episode turns (the common case today) produce byte-identical output to the pre-fix behavior — confirmed by the existing test suite's 0 changed assertions. The one intentionally changed existing assertion is ensureLeadingAssistantThinking's scope (latest-only → every tool_use turn), described above.

Linked Issues

Fixes #8258

中文说明

本 PR 做了什么

geminiChat.ts 的轮次整合逻辑会把一轮对话中所有带 thought 标记的部件合并成一个整体,并且只保留第一个出现的 thoughtSignature。当一轮对话包含多个独立的推理片段时——例如在 Anthropic 交替思考(interleaved thinking)或 OpenAI Responses 推理项中,每次并行工具调用都会对应一次推理——第一个之后的所有签名都会被静默丢弃,并且合并后的整体会被提前挪到该轮次的最前面,而不管这些推理片段相对于工具调用的实际发生顺序。

本 PR 用一套单遍算法替换了原来的"全部合并、只保留第一个签名"的逻辑:

  1. 当出现一个非 thought 部件时,或者当一个 thought 部件带来新的文本、而当前打开的片段已经同时具备累积文本和签名时,就结束(flush)当前的推理片段——这正是两个真正不同片段之间的边界。
  2. 在一个打开的片段内部,文本和签名片段会被拼接(而不是"只保留第一个见到的"),这样即使签名在多个流式数据块之间被拆分,也能被正确地重新组装,而不会被截断。

现在每个推理片段都会被保留为它自己的 Part,并保持在原始位置上,这与其他 TypeScript/Rust agent 框架表示多片段推理的方式一致(详见下方"证据"部分)。

在实现这个修复的过程中,还发现并一并修复了两个相关的 bug,因为如果不修复,它们会在特定情况下悄悄抵消这次主要修复:

  • XML 工具调用恢复兜底逻辑(Model outputs XML-style tool calls as plain text instead of structured function calls in long sessions #8003)此前是用一个简单的 .text !== undefined 判断来识别"需要被消费并替换为 remainingText 的文本部件"。由于一个推理片段的 Part 总是带有 .text 字段(即使是纯签名、没有文本内容的片段,其 .text 也会是空字符串),这个判断也会误命中推理片段——只要 XML 恢复逻辑在同一轮次里恰好和一个推理片段同时出现,就会把该推理片段的文本签名一并静默删除。修复方式是改用已有的 isValidNonThoughtTextPart 判断函数。
  • anthropicContentGenerator/converter.ts 中的 mergeConsecutiveAssistantMessages 在合并两条相邻的 assistant 消息时,会无条件地把所有 thinking 区块提到最前面,这会在需要合并两条 assistant 消息的那一刻,悄悄抵消本次修复所建立的按时间顺序排列。由于只要设置了 thinkinginterleaved-thinking-2025-05-14 就会被无条件启用,thinking 区块已经不需要再排在最前面,因此这里改为按原有顺序直接拼接。

Fixes #8258

证据 / 已有实践

我们查看了其他 agent 框架是如何在同一轮次内表示多个推理片段的——没有一个是把不同的片段合并成一个对象:

  • OpenAI Codexcodex-rs/protocol/src/models.rs:314-324)—— ResponseItem::Reasoning 对每一个推理片段都是一条独立的历史记录项。
  • OpenCodeanomalyco/opencodepackages/opencode/src/session/message-v2.ts)—— 每个推理片段都是它自己的 { type: "reasoning", text, metadata } 部件;该文件中还有一段明确的注释描述了 Anthropic 自适应思考下正是这种"一轮多片段"的形态。
  • Vercel AI SDKvercel/ai)—— LanguageModelV2ReasoningLanguageModelV2Content[] 数组中的一种变体;多个推理片段就是数组中的多个条目。

审阅者测试计划

如何验证

cd packages/core && npx vitest run src/core/geminiChat.test.ts src/core/anthropicContentGenerator/converter.test.ts

预期:两个文件中的全部测试通过(合计 369 个)。覆盖的关键场景包括:

  • 一轮对话中,推理片段与并行工具调用交替出现时,每个片段各自的签名都能被保留。
  • 没有中间工具调用、背靠背出现的两个推理片段,一旦第一个片段有了签名,仍然能被正确拆分。
  • 签名在多个流式数据块之间被拆分时,能正确拼接而不是被截断。
  • 一个推理片段与触发 XML 工具调用恢复(Model outputs XML-style tool calls as plain text instead of structured function calls in long sessions #8003)的轮次同时出现时,其文本和签名都能完整保留在历史记录中。
  • mergeConsecutiveAssistantMessages 在合并时保持按时间顺序排列,而不是把 thinking 区块提到最前面。

已知且已记录的残留限制(本 PR 未修复)

没有任何签名、且中间没有任何非 thought 部件、背靠背出现的两个 thought 部件,仍然会被合并成同一个片段——因为边界判断逻辑在没有签名可供比对的情况下无法识别出这是两个片段。这与两条链路都遵循的"每个片段都以一个纯签名数据块结尾"这一约定是一致的,并且按照目前的实现,这种情况在 Anthropic 交替思考或 OpenAI Responses 推理项中都不会真正出现;代码中已经加了注释说明这一点,以便将来如果某个不合规的代理完全丢弃了签名,方便定位。

风险与范围

  • 涉及的是所有链路(Gemini、Anthropic、OpenAI、OpenAI Responses)共用的历史整合代码,因此改动范围严格限定在 thought 部件的整合循环以及受其影响的两个相关调用点上,没有附带任何无关的重构。
  • feat(core): add OpenAI Responses API content generator #8169(OpenAI Responses API)相互独立——本 PR 不依赖、也不修改该 PR 引入的任何文件。
  • 破坏性改动 / 迁移说明:有一处刻意为之、此前未声明的改动。 recordAssistantTurn 现在会逐字记录所有整合后的部件,因此 inlineData/fileData 会写入会话 JSONL(旧逻辑对非 functionCall 部件返回 null,媒体部件从不落盘)。这对 --resume 的保真度是正确的取舍,但确实意味着模型产出的 base64 媒体会长期留在磁盘上,故在此显式说明。另外,ensureLeadingAssistantThinking 的作用范围已从「仅最后一条 assistant 消息」扩大到「每一条带 tool_use 的 assistant 消息」—— An error occurred when calling the DeepSeek v4 Pro model. #3786 针对的是先前的 assistant 轮次,且按位置归一化会导致同一轮次在相邻两次请求中序列化结果不同,从而每轮都打断 prompt cache 前缀。其余:单一片段的轮次(目前最常见的情况)的输出与修复前完全一致,字节级不变——现有测试套件中没有任何一条既有断言被修改,这一点可以印证。

关联 Issue

Fixes #8258

…y consolidation

geminiChat.ts's turn-consolidation step merged every thought-flagged
part in a turn into a single blob and kept only the first
thoughtSignature it saw. A turn with more than one distinct reasoning
episode -- e.g. one reasoning span per parallel tool call on Anthropic
interleaved thinking or OpenAI Responses reasoning items -- silently
lost every signature after the first, and hoisted the merged blob to
the front of the turn regardless of where the episodes actually
occurred relative to the tool calls.

Replace the merge-all/keep-first-signature pass with a single-pass
algorithm that treats a text-less, signature-only chunk as the natural
end of an episode on both wires, closes ("flushes") the current
episode when a non-thought part appears or when a thought part carries
fresh text while the open episode already has both text and a
signature, and concatenates (not "keeps only the first") text and
signature fragments within an open episode so a signature split across
multiple stream chunks is reassembled correctly. Each episode is now
preserved as its own history Part in its original position.

The XML-tool-call-recovery fallback (QwenLM#8003) had to be updated to match:
it previously identified "text parts to remove and replace with
remainingText" via a bare `.text !== undefined` check, which also
matched a reasoning episode Part (flushThoughtEpisode always sets
`episodePart.text`, even '' for a signature-only episode) -- silently
deleting the turn's reasoning episode, text and signature both,
whenever XML recovery fired on a turn that also carried one. Switched
to isValidNonThoughtTextPart, matching this path's actual intent.

mergeConsecutiveAssistantMessages in anthropicContentGenerator's
converter.ts had a related bug: it unconditionally hoisted all
thinking blocks to the front when merging two adjacent assistant
messages, which would silently undo the primary fix's chronological
ordering the moment two assistant messages needed merging. Since
interleaved-thinking-2025-05-14 is unconditionally enabled, thinking
blocks no longer need to lead; changed to straight concatenation.

Fixes QwenLM#8258.
@github-actions github-actions Bot added the review/self-reported The linked issue was opened by the PR author (self-reported) label Jul 31, 2026
@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

⚠️ Qwen Triage ended earlyview run. It stopped before finishing; check the run log.

⚠️ Qwen Triage 提前结束 —— 查看运行。未跑完,请查看运行日志。

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

Re-run at @wenshao's request, following his end-to-end verification. Gate re-checked at the current head.

Template looks good ✓

Problem: observed bug, now with wire-level evidence. #8258 surfaced in code review on #8169 with the root cause confirmed in source, and the maintainer's local harness (real CLI against a mock Anthropic endpoint) showed main does worse than drop the second signature — it emits one block carrying episode one's signature over both episodes' concatenated text, i.e. a mis-signed block. Observed, not theoretical.

Direction: aligned. Per-episode signature preservation is required for Anthropic interleaved thinking and OpenAI Responses replay validity. Claude Code's CHANGELOG ships a fix in the same problem area ("Fixed sessions getting stuck after ... stale thinking-block signatures in history").

Size: 589 production lines (geminiChat.ts +348/−84, converter.ts +130/−21, anthropicContentGenerator.ts +6/−0) and 2,519 test lines (+2,356/−163). Production grew from 225 lines at first triage past the 500 mark via review-round fixes (dangling-episode guards, predicate unification), so per the two-tier rule this is flagged for maintainer awareness — noted here, with the consequence stated in Stage 3. Not blocking on size alone.

Approach: still the minimal cohesive set. The primary consolidation change and its companions (XML-recovery predicate, converter hoist removal, dangling-episode guards) each close a defect that would otherwise undo the primary fix; splitting them would ship broken intermediate states. No drive-by changes.

Risk: ⚠️ geminiChat.ts matches this repo's high-risk path set (correlated with post-merge reverts), so full Stage 2 enrichment and CI evidence apply. The sandboxed /verify run is already in flight for this head (see Stage 2).

Moving on to code review. 🔍

中文说明

@wenshao 的要求,在其端到端验证之后重新运行。在当前 head 重新过门禁。

模板完整 ✓

问题:已观测到的 bug,现有线级证据。#8258#8169 的代码审查中被发现,根因已在源码中确认;维护者的本地验证环境(真实 CLI 对模拟 Anthropic 端点)进一步表明 main 的问题比"丢失第二个签名"更严重——它发出的块在两个片段的拼接文本上带着第一个片段的签名,即签名错误的块。已观测而非理论。

方向:一致。按片段保留签名对 Anthropic 交替思考与 OpenAI Responses 的重放有效性是必需的。Claude Code 的 CHANGELOG 发布过同一问题领域的修复("修复了……历史中残留的思考块签名导致会话卡住")。

规模:589 行生产代码geminiChat.ts +348/−84,converter.ts +130/−21,anthropicContentGenerator.ts +6/−0)与 2,519 行测试(+2,356/−163)。生产代码从首次分诊时的 225 行经审查轮次修复(悬空片段守卫、谓词统一)增长到超过 500 行,按两级规则标记给维护者知悉——在此标记,后果见 Stage 3。不单以规模阻塞。

方案:仍是最小内聚集合。主整合改动与伴随修复(XML 恢复谓词、converter 提前逻辑移除、悬空片段守卫)各自关闭一个会抵消主修复的缺陷;拆分会发布损坏的中间状态。无夹带改动。

风险:⚠️ geminiChat.ts 命中本仓库的高风险路径集(与合并后回滚相关),适用完整的 Stage 2 增强与 CI 证据。针对当前 head 的沙箱 /verify 运行已在进行中(见 Stage 2)。

进入代码审查 🔍

Qwen Code · qwen3.8-max

Reviewed at 6b3e68adc53b4b7a4b367e12cef76dae7cbe3a87 · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

Code review (at 6b3e68adc5, seventh-round head)

Independent proposal vs. the diff: I'd have done the same thing — replace merge-all/keep-first-signature with a single-pass episode tracker that flushes on a non-thought part or on fresh text after a signed episode, concatenate signature fragments, drop the converter's thinking hoist, and repair manual-mode's leading-thinking requirement uniformly across history so serialization stays position-independent. The PR matches this; I found no simpler path it missed. What it adds beyond my proposal — the dangling-episode guards and the predicate unification — each answers a concrete defect surfaced in the review rounds, not speculation.

What this pass verified, beyond the prior rounds:

  • The consolidation loop's boundary logic is correct, and the two documented known limitations are real and honestly bounded: back-to-back unsigned episodes merge (no signature to test), and two text-less signed episodes concatenate into one signature valid for neither — unreachable on the Anthropic wire, reachable only via the feat(core): add OpenAI Responses API content generator #8169 Responses shape with summaries disabled, and flagged there.
  • dropDanglingUnsignedTrailingThought's trailing-only scope is the right call: truncation can only strand the last episode, and non-signing providers (DeepSeek) legitimately carry unsigned thoughts mid-array. All four call sites are placement-justified; the XML-recovery one correctly captures trailing-ness before its removal loop (which would otherwise manufacture a trailing position), and the Math.min(insertAt, length) clamp covers the shrink.
  • isVisibleTextPart now makes contentText and the XML-removal loop the exact same set. I verified against main that isValidNonThoughtTextPart excludes thoughtSignature-bearing parts, so the deliberately looser predicate is the right middle: the stricter one would leak raw XML into durable history, the bare .text !== undefined one would delete reasoning episodes.
  • Converter: straight concatenation in mergeConsecutiveAssistantMessages is sound because interleaved-thinking-2025-05-14 is unconditionally enabled whenever thinking is set, and ensureLeadingThinkingOnToolUseAssistantMessages moves only the first contiguous thinking run, only on tool_use-bearing messages, only in manual mode — keeping the cached prefix stable across turns (wenshao confirmed byte-identical manual-mode output vs main, and prompt-cache prefix stability, on the wire).
  • Recording: redactStructuredOutputArgsForRecording returns null only for parts without a functionCall (checked against main), so the non-null assertion is valid; inlineData/fileData now reaching the session JSONL is declared in the PR body.

Findings: no critical blockers at this head. One accuracy nit for the record: the PR body says no existing assertion was modified, but one test's assertions were inverted — the converter merge-order test now expects [thinking, text, thinking, tool_use] instead of [thinking, thinking, text, tool_use]. That's the intentional behavior change itself, pinned by the rewritten test, and belongs with the maintainer's existing stale-count/doc nits. Non-blocking.

The consolidation → wire flow this PR reshapes:

sequenceDiagram
    participant P1 as Stream chunks
    participant P2 as Consolidation loop
    participant P3 as Dangling-episode guard
    participant P4 as History and JSONL
    participant P5 as Converter pipeline
    participant P6 as Anthropic wire
    P1->>P2: thought, text and toolCall parts in stream order
    P2->>P2: flush episode on non-thought, or on new text after a signature
    P2->>P3: each episode as its own Part, original position
    P3->>P4: drop unsigned trailing episode when a tool call is present
    P4->>P5: next request rebuilds messages from history
    P5->>P6: manual mode moves the first thinking run to the front
Loading
Files changed (6)
File What changed
packages/core/src/core/geminiChat.ts episode-tracking consolidation replacing merge-all/keep-first; shared visible-text predicate; dangling-episode guard at four sites; verbatim recording
packages/core/src/core/geminiChat.test.ts 26 new tests — multi-episode interleaving, split signatures, back-to-back episodes, XML-recovery interaction, recovery coalescing
packages/core/src/core/anthropicContentGenerator/converter.ts straight concatenation in assistant merge; new leading-thinking repair for manual mode; prefill strip moved ahead of the empty-text pass
packages/core/src/core/anthropicContentGenerator/converter.test.ts 12 new tests — merge ordering, leading-thinking scoping, position-independent serialization
packages/core/src/core/anthropicContentGenerator/anthropicContentGenerator.ts gates the new repair on the outgoing request's real manual-mode flag
packages/core/src/core/anthropicContentGenerator/anthropicContentGenerator.test.ts 3 end-to-end generator tests — manual explicit-budget, manual effort-ladder, adaptive stays chronological

Testing

Unattended CI run — no PR code executed locally. Evidence is the PR's own CI at the reviewed commit, quoted below, plus the maintainer's harness write-up (attributed) and the in-flight sandboxed run.

Check Conclusion
Test (ubuntu-latest, Node 22.x) ✅ success
Desktop Shell (ubuntu-22.04) ✅ success
Desktop Shell (windows-2022) ✅ success
web-shell E2E Smoke (ubuntu-latest, Node 22.x) ✅ success
Secret scan (TruffleHog) ✅ success
Dependency CVE audit ✅ success
precheck-pr / precheck ✅ success

All checks on 6b3e68adc5 are complete; none failed. Test (macos/windows) and Integration Tests (CLI) are skipped by design on every PR — those jobs run only in the merge queue, not conditional on ubuntu (correcting this comment's earlier characterization).

Behavioural evidence. The wire-level claim (episodes survive consolidation and replay byte-exact) is not settled by unit CI alone. Two things speak to it here: (1) wenshao's maintainer verification — real CLI against a mock Anthropic endpoint, A/B vs the merge-base, three mutation probes showing the new code is load-bearing, manual-mode output byte-identical to main — posted above and attributed to him, not re-run by this bot; and (2) a sponsored sandboxed /verify run is already in flight for this head (run 32659662886); its A/B report will post to the lifecycle comment. Read that report with the same skepticism as the fork's own CI logs — the code under verification is adversarial input even though the sandbox bounds what it can do.

Not verified by this run: real Anthropic API (mock only, by both harnesses), the OpenAI Responses wire (#8169), and the DeepSeek truncated-reasoning false positive (code-reading only).

中文说明

代码审查(在第七轮 head 6b3e68adc5 上)

独立方案对照: 我的独立方案与 diff 一致——单遍片段追踪器替换全部合并/只留第一个签名,边界时 flush,签名片段拼接,移除 converter 的 thinking 提前,并在全部历史上统一修复手动模式的首位 thinking 要求以保持序列化与位置无关。未发现被遗漏的更简路径。超出我方案的部分(悬空片段守卫、谓词统一)各自回应审查轮次暴露的具体缺陷,而非臆测加固。

本轮另外核实:整合循环的两个已知限制真实且边界诚实(背靠背无签名片段会合并;两个无文本带签名片段会拼成一个对两者都无效的签名——Anthropic 链路不可达,仅 #8169 关闭摘要的 Responses 形状可达,已在那边标记)。dropDanglingUnsignedTrailingThought 只看尾部是正确的:截断只会把最后一个片段悬空,非签名提供方(DeepSeek)中途带无签名 thought 是合法形状;四个调用点的位置都有论证,XML 恢复处在删除循环之前捕获尾部性,Math.min 钳位覆盖收缩。isVisibleTextPart 使 contentText 与 XML 删除循环成为同一集合;已对照 main 核实 isValidNonThoughtTextPart 会排除带 thoughtSignature 的部件,因此刻意放宽的谓词是正确的中间选择。converter 的直接拼接成立(interleaved-thinking-2025-05-14 在设置 thinking 时无条件启用),首位 thinking 修复只移动第一个连续 run、只作用于带 tool_use 的消息、只在手动模式——缓存前缀稳定(wenshao 在线上确认手动模式输出与 main 逐字节一致)。录制侧:已对照 main 核实 redactStructuredOutputArgsForRecording 仅对无 functionCall 的部件返回 null,非空断言成立;inlineData/fileData 落盘 JSONL 已在 PR 正文声明。

结论: 当前 head 无关键阻塞项。一条记录性小疵:PR 正文称"没有修改任何既有断言",但 converter 合并顺序测试的断言被反转([thinking, thinking, text, tool_use][thinking, text, thinking, tool_use])——这是有意的行为变更本身,由重写后的测试固定,与维护者已提的计数/文档小疵同类。不阻塞。

测试

无人值守 CI——未执行 PR 代码。证据为被审提交上 PR 自身 CI 的引用、维护者的验证记录(注明出处)与进行中的沙箱运行。

6b3e68adc5 上所有检查已完成,无失败。macOS/Windows 与集成测试在所有 PR 上按设计跳过——这些任务只在合并队列运行(更正本评论此前的表述)。

行为证据。 线级声明(片段在整合后存活且逐字节重放)不能仅由单测 CI 证实。此处有两项:(1) wenshao 的维护者验证——真实 CLI 对模拟 Anthropic 端点、与 merge-base A/B 对照、三个变异探针证明新代码承重、手动模式输出与 main 逐字节一致——已在上文发布,出自维护者而非本机器人复跑;(2) 针对该 head 的赞助沙箱 /verify 运行已在进行中,其 A/B 报告将发布到生命周期评论。请以与 fork CI 日志相同的怀疑态度阅读该报告。

本次运行未验证:真实 Anthropic API(两套环境均为 mock)、OpenAI Responses 链路(#8169)、DeepSeek 截断推理误报(仅代码阅读)。

Qwen Code · qwen3.8-max

Reviewed at 6b3e68adc53b4b7a4b367e12cef76dae7cbe3a87 · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

Confidence: 3/5 — clean review at this head, but the two-tier rule's 500+ production-line flag for fork core PRs caps the bot at defer; this is the policy speaking, not doubt about the code.

Stepping back: this is the strongest shape a fork PR can arrive in. The problem was observed in the wild (#8258), the root cause is confirmed in source, and the failure is actually worse than reported — a mis-signed block, not merely a lost signature. The algorithm is the natural one; I arrived at it independently before reading the diff. Seven review rounds converged — round seven raised only Suggestions, and wenshao's read of the thread is that no standing Critical remains at this head. The maintainer then verified it end-to-end on the wire with mutation probes and found the new code load-bearing and manual mode byte-identical to main. CI is green at 6b3e68adc5. If I were maintaining this in six months I'd thank the author — the comments explain the why of every guard, including the trade-offs each one accepts.

Why not approve, then: the PR now carries 589 production lines in packages/core/src/core/ (it was 225 at first triage; the growth is review-round fixes, which makes it legitimate, but the size fact stands). Under the two-tier core rule, a fork PR at that size gets maintainer awareness instead of the bot's automatic approval — and wenshao's awareness here, while unambiguously given, is exactly what the rule asks a human to own. He has already cast one of the two required approvals; the gate's question is whether the second one should be the bot's on a change this large in this file (geminiChat.ts is this repo's highest-revert-risk path). That is a human call, so I'm making it one explicitly rather than approving.

@wenshao — escalating per the size rule. Your options as I see them: a second human approval (yours already stands at 6b3e68adc5), or an explicit instruction in this thread to approve despite the size policy (a re-triggered /triage will read it as resolving the escalation), or merge via admin. Two housekeeping facts for whichever path you pick: the round-6 CHANGES_REQUESTED review from this bot account (commit 2fe2ee32, superseded by round 7) still pins reviewDecision and is dismissable by a maintainer; and the branch is 311 commits behind main — you validated the merged tree locally (no conflicts, suite green), so an up-to-date push should be clean, and any approval here is pinned to 6b3e68adc5 and dismisses on the push. The sandboxed /verify report will land in the lifecycle comment shortly.

@netbrah — nothing further needed from you on the code; this hold is policy, not findings.

中文说明

置信度:3/5 — 当前 head 上的审查是干净的;但两级规则对 fork 核心 PR 超过 500 行生产代码的标记将机器人限制为"转交"而非自动批准——这是规则在说话,不是对代码有疑虑。

退一步看:这是一个 fork PR 能达到的最强形态。问题在实际中被观测到(#8258),根因在源码中确认,而且实际失败比报告更糟——是签名错误的块,而非仅仅丢失签名。算法是自然的方案;我在读 diff 之前独立得出了同样的方案。七轮审查收敛——第七轮只有建议,维护者对线程的判断是该 head 上不再有未决 Critical。维护者随后在线上做了端到端验证并施加变异探针,确认新代码承重、手动模式与 main 逐字节一致。CI 在 6b3e68adc5 上全绿。六个月后维护这段代码我会感谢作者——每个守卫的注释都解释了"为什么",包括各自接受的取舍。

那为什么不批准:PR 现在携带 589 行生产代码,位于 packages/core/src/core/(首次分诊时是 225 行;增量来自审查轮次修复——这使其正当,但规模事实不变)。按两级核心规则,该规模的 fork PR 获得的是维护者知悉,而不是机器人的自动批准——wenshao 的知悉在这里毫无歧义,但规则要求的正是由来拥有这个决定。他已经投出两个必需批准中的一个;门禁的问题是:在最高回滚风险路径(geminiChat.ts)上这么大的改动,第二个批准是否应该来自机器人。这是人的决定,所以我把它明确交出去,而不是自行批准。

@wenshao —— 按规模规则转交。我看到的路径:第二个人类批准(你的已落在 6b3e68adc5),或在本线程明确指示忽略规模政策批准(重新触发的 /triage 会将其读作升级已解决),或以管理员身份合并。无论哪条路径,两个事务性事实:本机器人账号第六轮的 CHANGES_REQUESTED 审查(提交 2fe2ee32,已被第七轮取代)仍钉住 reviewDecision,维护者可将其驳回;分支落后 main 311 个提交——你已在本地验证过合并树(无冲突、套件全绿),同步推送应该是干净的,且此处的任何批准都钉在 6b3e68adc5 上、推送即失效。沙箱 /verify 报告稍后会落在生命周期评论里。

@netbrah —— 代码层面无需再做任何事;本次转交是政策原因,不是发现问题。

Qwen Code · qwen3.8-max

Reviewed at 6b3e68adc53b4b7a4b367e12cef76dae7cbe3a87 · re-run with @qwen-code /triage

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, looks ready to ship — CI landed green after the review. ✅

@netbrah

netbrah commented Jul 31, 2026

Copy link
Copy Markdown
Contributor Author

Live wire verification: same-response multi-episode replay confirmed

I ran the built dogfood/reasoning-fidelity branch through a byte-recording reverse proxy against a real OpenAI Responses model (gpt-5.6-sol, high reasoning effort). This produced the exact runtime shape that the consolidation fix targets, not merely multi-turn signature growth.

Qualifying response

One SSE response completed two distinct encrypted reasoning items before its function call. Sanitized response.output_item.done events:

{"output_index":0,"type":"reasoning","id":"rs_040c2d905c1da525016a6d12a1e9108197bbaf5185bd010f22","encrypted_bytes":1464,"encrypted_content_sha256":"cc02d44cb4fdbd13d9f52df07f02ec293561fa63ac42a1a9fd7a234e54c31e7a"}
{"output_index":1,"type":"reasoning","id":"rs_040c2d905c1da525016a6d12a3b08c8197afebe810eb1c207f","encrypted_bytes":1100,"encrypted_content_sha256":"eb592d9b5256e58fb86bd034029823cfa8be61cc7ea54f8efec60f5f038f4bd8"}
{"output_index":2,"type":"function_call","id":"fc_040c2d905c1da525016a6d12a4cd3481978ff7576b94ac7264","call_id":"call_RCmWggzme2vihVKSZ7Ql4UZq","name":"run_shell_command"}

So the model emitted this in one response:

reasoning A + encrypted payload A
reasoning B + encrypted payload B
function_call

This is the PR's back-to-back episode case: a second reasoning episode begins after the first episode is signed, without a tool-call boundary between them.

Immediately following request

After Qwen Code converted and consolidated that response, the next request contained these ordered replay items:

{"input_index":1,"type":"reasoning","id":"rs_040c2d905c1da525016a6d12a1e9108197bbaf5185bd010f22","encrypted_bytes":1464,"encrypted_content_sha256":"cc02d44cb4fdbd13d9f52df07f02ec293561fa63ac42a1a9fd7a234e54c31e7a"}
{"input_index":2,"type":"reasoning","id":"rs_040c2d905c1da525016a6d12a3b08c8197afebe810eb1c207f","encrypted_bytes":1100,"encrypted_content_sha256":"eb592d9b5256e58fb86bd034029823cfa8be61cc7ea54f8efec60f5f038f4bd8"}
{"input_index":3,"type":"function_call","call_id":"call_RCmWggzme2vihVKSZ7Ql4UZq","name":"run_shell_command"}
{"input_index":4,"type":"function_call_output","call_id":"call_RCmWggzme2vihVKSZ7Ql4UZq"}

The verification compared the complete opaque payloads, not truncated display strings:

response reasoning IDs   == next-request reasoning IDs       PASS
response payload lengths == next-request payload lengths     PASS
response SHA-256 list     == next-request SHA-256 list        PASS
response item order       == next-request replay order        PASS
Episode Response bytes Next-request bytes Response SHA-256 Next-request SHA-256 Result
A 1464 1464 cc02d44cb4fdbd13d9f52df07f02ec293561fa63ac42a1a9fd7a234e54c31e7a same exact
B 1100 1100 eb592d9b5256e58fb86bd034029823cfa8be61cc7ea54f8efec60f5f038f4bd8 same exact

This is direct live-wire evidence that both same-response reasoning episodes survived geminiChat.ts history consolidation as separate entries, retained their order relative to the function call, and replayed byte-for-byte. Under the previous merge-all/keep-first behavior, episode B's replay payload would have been lost.

A separate four-user-turn Anthropic capture verified ordinary history accumulation (0,1,1,2,2,3,3,4) with every signature list an ordered byte-exact prefix of the final list, but that is only a replay-plumbing baseline. The Responses capture above is the load-bearing proof for this PR because both encrypted episodes originated in one model response.

I also probed claude-sonnet-5 with adaptive thinking, high effort, and interleaved thinking confirmed on the wire. Sonnet produced separate HTTP responses (thinking -> tool_use per tool round), not multiple thinking blocks inside one response. That is consistent with Anthropic's documented tool loop: interleaved thinking occurs after tool results arrive on subsequent API calls. The OpenAI Responses capture independently reached the exact shared consolidation path fixed here.

The later test-session stop (Model stream ended after a tool result without visible progress) occurred after the qualifying response/request transition and does not affect the byte-exact consolidation proof above.

中文说明

真实链路验证:已确认同一响应中的多个推理片段可无损重放

我将构建后的 dogfood/reasoning-fidelity 分支通过字节级记录代理连接到真实 OpenAI Responses 模型(gpt-5.6-sol,高推理强度)。本次运行生成了该整合修复真正针对的运行时形状,而不仅仅是跨多轮的签名增长。

符合条件的响应

同一个 SSE 响应先完成了两个不同的加密推理 item,然后才输出 function call。上方英文部分的 JSONL 是脱敏后的真实事件,顺序为:

推理片段 A + 加密载荷 A
推理片段 B + 加密载荷 B
function_call

这正是本 PR 的“背靠背推理片段”场景:第一个片段完成签名后,第二个片段开始;两者之间没有工具调用作为天然边界。

紧接着的下一次请求

Qwen Code 转换并整合该响应后,下一次请求按顺序包含:

input[1] = reasoning A
input[2] = reasoning B
input[3] = function_call
input[4] = function_call_output

验证比较的是完整不透明载荷,而不是截断字符串:

响应中的 reasoning ID   == 下一请求中的 reasoning ID       通过
响应中的载荷长度          == 下一请求中的载荷长度              通过
响应中的 SHA-256 列表     == 下一请求中的 SHA-256 列表         通过
响应中的 item 顺序        == 下一请求中的重放顺序               通过
片段 响应字节数 下一请求字节数 SHA-256 结果
A 1464 1464 cc02d44cb4fdbd13d9f52df07f02ec293561fa63ac42a1a9fd7a234e54c31e7a 完全一致
B 1100 1100 eb592d9b5256e58fb86bd034029823cfa8be61cc7ea54f8efec60f5f038f4bd8 完全一致

这是直接的真实链路证据:同一个模型响应中的两个推理片段都以独立条目通过了 geminiChat.ts 历史整合,保留了相对于 function call 的顺序,并逐字节重放。旧的“全部合并、只保留第一个签名”行为会丢失片段 B 的可重放载荷。

另一次 Anthropic 四用户轮次捕获验证了普通历史累积(0,1,1,2,2,3,3,4),且每次请求的签名列表都是最终列表的有序、逐字节一致前缀;但这只是重放链路基线。上面的 Responses 捕获才是本 PR 的关键证据,因为两个加密片段来自同一个模型响应。

我还使用 claude-sonnet-5 验证了 Anthropic 链路,并确认真实请求启用了 adaptive thinking、高 effort 和 interleaved thinking。Sonnet 的实际行为是每个工具轮次使用独立 HTTP 响应(thinking -> tool_use),没有在同一个响应中生成多个 thinking 块。这与 Anthropic 文档描述的工具循环一致:收到工具结果后,模型在后续 API 请求中继续交替思考。OpenAI Responses 的真实捕获已经独立到达本 PR 修复的共享整合路径。

后续测试会话出现的 Model stream ended after a tool result without visible progress 发生在上述合格的响应/请求转换之后,不影响上面的逐字节整合证明。

qqqys
qqqys previously requested changes Jul 31, 2026

@qqqys qqqys left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A blocking manual-thinking regression remains in the converter merge path. The inline finding identifies the deterministic request shape and affected model mode.

Comment thread packages/core/src/core/anthropicContentGenerator/converter.ts
Addresses review feedback on QwenLM#8260: mergeConsecutiveAssistantMessages's
straight concatenation preserves chronological order but can leave a
merged assistant turn's content beginning with text instead of
thinking (e.g. [text A] + [thinking B, tool_use B] -> [text A,
thinking B, tool_use B]). Anthropic's manual (non-adaptive)
extended-thinking contract requires the final assistant turn of a
thinking-enabled request to begin with a thinking block whenever a
tool_use remains in it; adaptive thinking has no such requirement, so
the request would 400 on the follow-up tool-result turn.

Add ensureLeadingAssistantThinking, a converter option gated on the
outgoing request's actual thinking.type === 'enabled' mode (passed
from anthropicContentGenerator.ts). When set, after all merge/cleanup
passes finish, it relocates the most recent assistant message's first
contiguous thinking/redacted_thinking run to the front of its content
array -- and only that run, leaving every other block (including
later thinking blocks and their relative order) untouched. It does
not fabricate a thinking block where none exists, and is a no-op for
adaptive-thinking models (Opus 4.7+, every 5.x) and thinking-off
requests.

Replaces the previous "pins current behavior" regression test (which
documented the residual risk without fixing it) with two assertions:
adaptive/default mode still preserves chronological order, and the
new option produces the required leading-thinking shape. Adds a
generator-level regression test using an explicit-budget (manual)
configuration on claude-opus-4-6, covering the full tool-loop request
shape, the interleaved-thinking beta, and signature preservation.

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed — no blockers. Suggestions are inline.

中文说明

已审查——无阻断问题。 建议见行内评论。

— qwen3.8-max-preview via Qwen Code /review

Comment thread packages/core/src/core/geminiChat.ts Outdated
Comment thread packages/core/src/core/anthropicContentGenerator/converter.test.ts
@wenshao

wenshao commented Aug 1, 2026

Copy link
Copy Markdown
Collaborator

Code Review — fix(core): preserve every reasoning episode's signature during history consolidation

Reviewed at b7fe21c (base 912f7399). I ran the three touched suites locally — 486/486 pass (geminiChat.test.ts 278, anthropicContentGenerator.test.ts 116, converter.test.ts 92).

Overview

The core change is right and well-argued. Replacing the merge-all/keep-first-signature pass with a single-pass episode tracker is the correct shape: each reasoning episode keeps its own Part, its own signature, and its original position relative to the tool calls it preceded. The mergeConsecutiveAssistantMessages de-hoisting and the isValidNonThoughtTextPart fix in the XML-recovery path are genuinely coupled to it — splitting them would leave intermediate broken states. Comment density and the "known limitation" callout are exemplary; the tests pin the interesting boundaries (interleaved episodes, back-to-back episodes, fragmented signature, signature-only mid-turn episode, OpenAI-Responses-shaped payloads).

Two things need attention before merge, one of them a verified regression.


1. 🔴 Regression: XML recovery now leaves raw <invoke> XML in history for { text, thoughtSignature } parts

geminiChat.ts uses two different predicates for what is supposed to be the same set of parts:

  • contentText (and therefore recovery.remainingText) — part.text && !part.thought
  • the removal loop — isValidNonThoughtTextPart(part), which additionally rejects part.thoughtSignature, inlineData, fileData, functionCall, functionResponse

They are no longer complements. A non-thought text part carrying a thoughtSignature — Gemini's placement for the signature that concludes a reasoning span, and a shape this repo explicitly expects (loggingContentGenerator.ts:976-986 preserves exactly { text, thoughtSignature } with no thought flag) — is counted into contentText but not removed. The result: the original part survives with the raw XML in it, and remainingText is spliced in on top → duplicated prose and <invoke …> leaking into durable history.

Verified with a probe (single chunk, parts [{ text: 'Sure.\n<invoke …>', thoughtSignature: 'gemini-sig' }], then assert no history part contains <invoke):

commit result
912f7399 (base) ✅ passes — old .text !== undefined consumed the part
b7fe21c (this PR) ❌ fails — history is [{text:'Sure.'}, {text:'Sure.\n<invoke …>', thoughtSignature}, {functionCall}]

The existing test retains a short text prefix in history when recovering XML tool calls asserts exactly this invariant; it just doesn't cover the signature-bearing variant.

Suggested fix — one predicate, used in both places:

const isConsumableRecoveryText = (part: Part) =>
  !part.thought && typeof part.text === 'string' && part.text !== '';

…and use it for contentText, for the recomputed contentText, and for textIndices. (Or keep isValidNonThoughtTextPart for both — either way they must agree.) Worth a regression test with thoughtSignature on the plain-text part.

2. 🟡 Undeclared behavior change: session JSONL now embeds inlineData/fileData blobs

The recording rewrite changed more than the reasoning parts. Old code built the record explicitly ([thought?, {text: contentText}?, ...functionCallParts]) — redactStructuredOutputArgsForRecording returned null for every non-functionCall part, so media parts were never recorded. New code records every element of consolidatedHistoryParts.

Verified probe (model turn = [{text:'here is the image'}, {inlineData:{mimeType:'image/png', data:'BASE64BLOB'}}]):

base      PROBE RECORDED [{"text":"here is the image"}]
this PR   PROBE RECORDED [{"text":"here is the image"},{"inlineData":{"mimeType":"image/png","data":"BASE64BLOB"}}]

For image-capable models this writes full base64 payloads into the session JSONL on every turn — unbounded file growth, and model-produced media now sits on disk indefinitely, which reads against the privacy rationale documented right above redactStructuredOutputArgsForRecording. The PR states "Breaking changes / migration notes: none", so this looks unintended rather than a deliberate fidelity improvement. Either filter the recording to text/thought/functionCall parts, or make it an explicit, documented decision.

While you're there: .filter((part): part is NonNullable<typeof part> => part !== null) after part.functionCall ? redact(part) : part is dead — redactStructuredOutputArgsForRecording only returns null when !part.functionCall, which the ternary already excludes.

3. 🟡 Commit 2 (ensureLeadingAssistantThinking) isn't in the PR description

The description only covers the de-hoisting; the second commit partially re-introduces it for manual mode. That's a reasonable escape hatch, but three things:

  • Update the PR body — a reviewer reading only the description will not know this pass exists.
  • What's the evidence? The added test asserts our own converter output against a mocked SDK, so it can't demonstrate that Anthropic actually rejects the straight-concatenated shape. Did you see a live 400 (Expected 'thinking' or 'redacted_thinking', but found 'text') on a manual-budget model? If yes, quoting it in the body would settle it; if it's precautionary, say so.
  • Scope: the doc says "whenever a tool_use remains in it", but the implementation reorders the latest assistant message unconditionally. When that message has no tool_use (e.g. a fresh user turn following a completed assistant turn), the reorder is unnecessary and re-breaks the chronology this PR set out to protect. Gating on blocks.some(b => b.type === 'tool_use') would make it match its own doc.

4. 🟢 Follow-ups / observations (non-blocking)

  • Per-episode .trim() vs. signature validity. flushThoughtEpisode trims each episode's accumulated text. Anthropic's signature is computed over the exact thinking text, so trimming is a replay-validity hazard. It's pre-existing for the single-episode case, but the multi-episode split now applies it at every internal episode boundary too (e.g. "A\n\n" + " B" used to join to "A\n\n B", now becomes "A" and "B"). Consider keeping the raw text on the Part and using the trimmed value only for the "is this episode empty" test.
  • redactStructuredOutputArgsForRecording still drops thoughtSignature from functionCall parts (return { functionCall: part.functionCall }). Since Gemini attaches the signature to the functionCall part, --resume still loses signatures on that wire — the same class of bug this PR fixes for thought parts. Worth a follow-up issue.
  • The boundary heuristic reconstructs information the wire already had. anthropicContentGenerator.ts sees content_block_start / content_block_stop with an explicit index for every thinking block, and currently emits nothing for them. Threading a real boundary signal through (block index on the chunk, or a synthetic episode-close part) would eliminate both the documented no-signature limitation and the [sig, text, sig] case, where openEpisodeText.length > 0 suppresses the flush and two distinct signatures get concatenated into one unusable blob. Given the effort already spent on the heuristic, an explicit boundary looks like the cheaper long-term shape.
  • Aliasing nit. this.history.push({ role: 'model', parts: consolidatedHistoryParts }) now shares the live array (previously a fresh spread). Nothing mutates it after this point today; a [...consolidatedHistoryParts] is cheap insurance against a future edit below the push.

Verdict

Direction and core algorithm: approve. Item 1 is a blocking regression — a verified behavior change from base that puts raw tool-call XML back into durable history. Item 2 needs an explicit decision (fix or document). Item 3 is a description/scoping gap. Items in §4 are follow-ups.

中文摘要

b7fe21c(base 912f7399)上审阅,本地跑通三个测试文件共 486/486 通过

主体算法是对的:用单遍的“推理片段”跟踪替换掉“全部合并、只留第一个签名”,每个片段保留自己的 Part、自己的签名和相对工具调用的原始位置;配套的 de-hoisting 和 XML 恢复谓词修复确实与主修复耦合,不应拆分。注释质量和“已知限制”的说明都很好。

合并前需要处理两点(其一为已验证的回归):

  1. 🔴 回归:XML 恢复会把原始 <invoke> 留在历史里。 contentTextpart.text && !part.thought 筛选,删除循环却用 isValidNonThoughtTextPart(额外排除带 thoughtSignature 的部件)。对于 { text, thoughtSignature }thought 为假的部件——正是 Gemini 的签名放置方式,loggingContentGenerator.ts:976-986 明确保留这种形状——它会被计入 contentText 却不会被删除,导致文本重复且原始 XML 残留在持久化历史中。探针在 base 通过、在本 PR 失败。建议两处统一使用同一个谓词,并补一个带 thoughtSignature 的回归测试。
  2. 🟡 未声明的行为变更:JSONL 现在会记录 inlineData/fileData 旧代码只记录 thought/文本/functionCall(redactStructuredOutputArgsForRecording 对非 functionCall 返回 null),新代码记录全部部件。探针确认:base 只记录 [{"text":...}],本 PR 还会写入完整 base64。对图像模型意味着会话文件无限膨胀、模型产出的媒体长期落盘,与该函数上方记录的隐私约定相悖;而 PR 声明“无破坏性改动”。请显式过滤或明确记录该决定。另:.filter(part => part !== null) 是死代码。
  3. 🟡 第二个 commit(ensureLeadingAssistantThinking)未写进 PR 描述,它在 manual 模式下部分恢复了第一个 commit 所反对的重排。请补充描述、说明证据(新增测试只断言我方转换结果,无法证明 Anthropic 确实会 400),并考虑按文档所述加上 tool_use 判断(当前实现是无条件重排最后一条 assistant 消息)。
  4. 🟢 后续可跟进:逐片段 .trim() 会改变 thinking 原文、可能影响签名校验;redactStructuredOutputArgsForRecording 仍会丢掉 functionCall 上的 thoughtSignature(Gemini 链路的 --resume 仍丢签名);边界启发式其实可以由 content_block_start/stop 提供显式边界来彻底解决;this.history.push 现在与本地数组共享引用,建议展开复制。

结论:方向与算法认可;第 1 点为阻塞性回归,第 2 点需要明确决策,第 3 点是描述/范围问题。

@wenshao

wenshao commented Aug 1, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /resolve

@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

Qwen Code resolved the merge conflicts and pushed the branch update.

Merge resolution: PR #8260 ← main

Root cause

Both this PR and main inserted a brand-new helper at the same anchor point in packages/core/src/core/anthropicContentGenerator/converter.ts — the blank line between mergeConsecutiveAssistantMessages and the cleanOrphanedToolCalls doc comment.

Git aligned on the shared /** … } skeleton there and reported one conflict (~lines 1439–1516).

Textual, not semantic

The sides share no logic. Each added an independent, self-contained function with its own doc comment. Resolution: keep both verbatim, PR's first then main's, each with its own closing brace. Everything else auto-merged — main's dropEmptyTextThinkingBlocks step and its two makeToolResultDeduper() call sites (in cleanOrphanedToolCalls and mergeConsecutiveUserMessages) landed cleanly beside the PR's ensureLeadingThinkingOnLatestAssistantMessage(messages) call.

What is load-bearing

  • Both helpers must stay defined — each has live callers. ensureLeadingThinking… is called once (guarded by options.ensureLeadingAssistantThinking, last in the pipeline after mergeConsecutiveUserMessages/stripTrailingAssistantPrefill); makeToolResultDeduper is called twice. Dropping either to "resolve" the conflict leaves a dangling reference.
  • Pipeline ordering is untouched by this merge. The PR's leading-thinking fix runs last, after main's new dropEmptyTextThinkingBlocks; main's comment requires dropUnsignedThinkingFromAssistantMessages to run before dropEmptyTextThinkingBlocks. A future edit reordering these three steps breaks that documented invariant.

What I could not verify

No build/typecheck/tests run here (out of scope). The merge is textually additive, so I expect no behaviour change beyond the union of both branches' effects. Watch in CI: both sides edit converter.test.ts (auto-merged, no conflict), but the combined pipeline now holds both dropEmptyTextThinkingBlocks and ensureLeadingThinking… — any assertion on pipeline count/order runs against that new reality.

中文说明

根因

本 PR 与 main 都在 converter.ts同一插入点mergeConsecutiveAssistantMessagescleanOrphanedToolCalls 文档注释之间)新增了辅助函数:PR #8260 加了 ensureLeadingThinkingOnLatestAssistantMessage;main 提交 40efe41ce#8163)加了 makeToolResultDeduper。Git 围绕该处共享的 /** … } 骨架对齐,报出一处冲突。

纯文本冲突,非语义冲突

两侧不共享任何逻辑,各自新增独立、自包含、带独立文档注释的函数。解决方式:原样保留两个函数,PR 的在前、main 的在后,各自带闭合花括号。其余部分自动合并——main 的 dropEmptyTextThinkingBlocks 流水线步骤及其两处 makeToolResultDeduper() 调用点干净地落在 PR 的调用旁边。

关键承重点

  • 两个辅助函数都必须保留定义,各自都有活跃调用方;删掉任何一个都会留下悬空引用。
  • 流水线顺序未被本次合并改动:PR 的 leading-thinking 修正在最后运行,位于 main 新增的 dropEmptyTextThinkingBlocks 之后;main 的注释要求 dropUnsignedThinkingFromAssistantMessages 先于 dropEmptyTextThinkingBlocks。未来重排这三步会破坏该不变量。

无法验证的部分

本命令不运行 build/typecheck/测试。合并在文本上纯叠加,预期行为变化仅为两分支效果之并集。CI 中需留意:两侧都改了 converter.test.ts(自动合并无冲突),但合并后的流水线同时包含 dropEmptyTextThinkingBlocksensureLeadingThinking…,关于流水线数量/顺序的断言将以这一新现实为准。

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] XML-recovery predicate divergence leaks raw tool-call XML into durable history (maintainer @wenshao's item-1 blocker, re-verified at this commit by probe). contentText (geminiChat.ts:4327 and its recomputation at :4385) selects parts via part.text && !part.thought, which INCLUDES a {text, thoughtSignature} part carrying no thought flag — a real Gemini wire shape (loggingContentGenerator.ts:1004-1009 preserves exactly that shape and adds thought:true only when part.thought is truthy). But the removal loop (geminiChat.ts:4366) uses isValidNonThoughtTextPart, which additionally rejects thoughtSignature. The two predicates are not complements. When such a part's text contains raw <invoke> XML tool calls, recovery fires (contentText includes the part) yet the part is NOT removed (the removal loop excludes it): for a single such part textIndices is empty, nothing is removed, remainingText is spliced in at index 0, and the original part survives with the raw XML in history — duplicated prose plus a tool-call XML leak. Probe-confirmed at 2a76e3e: current code yields [{text:'Sure.'}, {text:'Sure.\n<invoke …>', thoughtSignature}, {functionCall}]; aligning the two predicates fixes it. Fix: use one predicate (e.g. isValidNonThoughtTextPart) for contentText, its recomputation, and textIndices, and add a regression test with thoughtSignature on a plain-text part that carries XML.

— qwen3.8-max-preview via Qwen Code /review

Comment thread packages/core/src/core/geminiChat.ts Outdated
Addresses a Critical from PR QwenLM#8260 review (and the related "three
uncoordinated predicates" Suggestion it escalated): contentText's
filter (`part.text && !part.thought`) and the XML-recovery removal
loop's filter (`isValidNonThoughtTextPart`, which additionally rejects
any part carrying `thoughtSignature`) disagreed on what counts as
"visible text." A part with `thoughtSignature` set but no `thought:
true` -- a real wire shape (loggingContentGenerator.ts's stream
aggregation spreads `thought` and `thoughtSignature` independently) --
was picked up by contentText for XML detection but survived the
removal loop untouched: recovery fired, but the raw `<invoke>` XML was
never stripped, leaking it into durable history duplicated alongside
the recovered functionCall.

Introduce a single `isVisibleTextPart` predicate (`Boolean(part.text)
&& !part.thought`) shared by contentText's initial computation, its
post-recovery recompute, and the removal loop's textIndices scan.
Deliberately the looser of the two prior predicates, not the stricter
one: narrowing contentText itself to exclude thoughtSignature-bearing
text would make `hasAnyContent` treat genuine visible text as absent,
throwing "Model stream ended with empty response text" on ordinary
turns. flushThoughtEpisode always sets `thought: true` on episode
parts, so `!part.thought` alone (already contentText's semantics)
already protects reasoning episodes from the removal loop without
isValidNonThoughtTextPart's stricter signature exclusion.

Adds a regression test that reproduces the leak on unfixed code
(confirmed failing before this fix, passing after) with a plain-text
part carrying a stray thoughtSignature and XML content.

Also addresses two outstanding test-coverage Suggestions from the same
review round:
- converter.test.ts: a multi-thinking-run case for
  ensureLeadingAssistantThinking, guarding the "only the first run
  moves" invariant against a hoist-all-thinking mutant that the
  existing single-run test couldn't catch.
- anthropicContentGenerator.test.ts: a generator-level adaptive-mode
  test mirroring the manual-mode one, guarding the `thinking?.type ===
  'enabled'` gate against a `!!thinking` regression that would
  reintroduce the hoist-every-thinking corruption on adaptive models.
- geminiChat.test.ts: asserts the interleaved-episode test's recorded
  JSONL turn (not just in-memory history) preserves both reasoning
  episodes and their signatures, guarding --resume fidelity against a
  recording-only regression that in-memory assertions can't see.
@netbrah
netbrah requested a review from doudouOUC as a code owner August 3, 2026 19:44
@netbrah

netbrah commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the Critical from the latest review round ("XML-recovery predicate divergence leaks raw tool-call XML into durable history"), fixed in da4b3e381.

Confirmed the finding by reproduction before fixing: contentText's filter (part.text && !part.thought) and the XML-recovery removal loop's filter (isValidNonThoughtTextPart, which additionally excludes any part carrying thoughtSignature) disagreed on what counts as "visible text." A part with thoughtSignature set but no thought: true — a real wire shape (loggingContentGenerator.ts's stream aggregation spreads thought and thoughtSignature independently) — was picked up by contentText for XML detection but survived the removal loop untouched, leaking the raw <invoke> XML into history duplicated alongside the recovered functionCall.

Fix: a single isVisibleTextPart predicate now backs contentText's initial computation, its post-recovery recompute, and the removal loop's textIndices scan — the same fix requested by an earlier Suggestion on this PR flagging the three-way predicate duplication. Deliberately kept the looser predicate (not isValidNonThoughtTextPart) as the shared one: narrowing contentText itself would make hasAnyContent treat genuine visible text as absent on ordinary turns. flushThoughtEpisode always sets thought: true on episode parts, so !part.thought alone (already contentText's semantics) was always sufficient to protect reasoning episodes from the removal loop — the extra signature exclusion in isValidNonThoughtTextPart was unneeded there and is what caused the divergence.

Added a regression test with a plain-text part carrying a stray thoughtSignature and XML content — confirmed it fails on unfixed code (raw XML survives in history) and passes after the fix.

Also landed the two outstanding test-coverage Suggestions from the same review round (multi-thinking-run case for ensureLeadingAssistantThinking, and a generator-level adaptive-mode test), plus a JSONL-recording regression test for interleaved reasoning episodes. All addressed inline threads are marked resolved.

Full suite: 500 passing / 1 pre-existing unrelated failure (a User-Agent header assertion that fails identically on origin/main due to an environment variable in this sandbox, confirmed unrelated to this change). Typecheck and lint clean.

One item for visibility: this branch was recently updated with a merge of main, but main has since advanced enough that GitHub now reports mergeStateStatus: CONFLICTING (conflict in geminiChat.ts against the latest main). Flagging rather than resolving unprompted, since geminiChat.ts is high-traffic and I'd rather have alignment before rebasing again.

Comment thread packages/core/src/core/geminiChat.ts Outdated
Comment thread packages/core/src/core/anthropicContentGenerator/converter.ts Outdated
Comment thread packages/core/src/core/geminiChat.ts Outdated
Comment thread packages/core/src/core/geminiChat.ts Outdated
Comment thread packages/core/src/core/geminiChat.ts Outdated
Comment thread packages/core/src/core/geminiChat.ts Outdated
Comment thread packages/core/src/core/anthropicContentGenerator/converter.ts
Palanisamy, Dinesh added 3 commits August 3, 2026 21:04
…t-consolidation

# Conflicts:
#	packages/core/src/core/geminiChat.ts
…urns

Addresses the Critical from PR QwenLM#8260's latest review round, verified by
tracing the actual code before fixing (not taken on the reviewer's
word): flushThoughtEpisode's own "Known limitation" note already
acknowledged that a stream cut off before an episode's terminating
signature-only chunk arrives (SSE drop, MAX_TOKENS) leaves that
episode unsigned. Left in history alongside a tool_use in the SAME
turn, this permanently wedges a session: once the tool result is
appended, the turn enters dropUnsignedThinkingFromAssistantMessages's
"active tool-use chain" and every subsequent request throws on
proxy-hosted adaptive Claude (native Anthropic rejects the unsigned
block itself instead) -- neither is recoverable without editing
history out-of-band, since the malformed turn is now a permanent part
of the session's history.

Fix: after the trailing flushThoughtEpisode() call, if the turn has a
tool_use and the last consolidated part is an unsigned trailing
thought episode, drop it before it can ever reach history. Scoped to
`hasToolCall` because a dangling unsigned episode with no tool_use in
the same turn is already filtered out safely downstream (it never
enters the active-chain path). Added a regression test reproducing the
exact wedge scenario, confirmed failing on unfixed code (the unsigned
episode survived in history) and passing after.

Also addresses four Suggestions from the same review round, each
verified against the actual code (one live-mutated to confirm it
catches what's claimed) rather than accepted at face value:
- converter.ts: added a test with two non-consecutive assistant
  messages (separated by a user turn) to discriminate
  ensureLeadingAssistantThinking's backward scan from a forward-scan
  mutation that reorders the wrong turn -- confirmed by temporarily
  applying the mutation and observing exactly this new test fail.
- converter.ts: added a multi-block first-thinking-run test to
  discriminate the run-extension loop from a `runEnd = runStart + 1`
  mutation that would split a multi-block run apart -- confirmed the
  same way.
- geminiChat.ts: added two tests for the episode-split condition's
  `openEpisodeText.length > 0` and `openEpisodeSignature !== ''`
  clauses (signature arriving before any text; multiple text deltas
  within one still-open episode, the normal live-streaming shape) --
  each confirmed to fail when its corresponding clause is removed.
- geminiChat.ts / converter.ts: fixed a stale comment (the JSONL
  recording no longer reads the recomputed contentText, it reads
  consolidatedHistoryParts directly) and qualified
  dropEmptyTextThinkingBlocks's doc, which read as contradicting
  flushThoughtEpisode's "still potentially replayable" rationale for
  the same empty-text+signature shape -- clarified that the two are
  consistent (disposability of non-latest-turn thinking, not
  invalidity of the shape itself), with a cross-reference each way.

One Suggestion from the same round was checked and found NOT to hold:
a claim that no test pins flushThoughtEpisode's "drop a whitespace-only,
signature-less episode" guard. Forcing that guard to unconditionally
true and running the full suite shows this is false -- "should
preserve text parts that stream in the same chunk as a thought" (an
existing test) goes red under exactly that mutation. No change made
for this one.
…e dangling-episode fix

A scoped multi-model architectural review of the reasoning-episode
consolidation logic (3 independent reviewers, one per major model
family) converged on the same Critical finding, plus a second real
gap and a lower-priority structural one. Every finding was
independently re-verified against the actual code (traced by hand,
or confirmed/refuted via live mutation) before acting -- one line of
investigation that looked promising turned out to cause a real
regression and was redesigned rather than shipped as-is (see below).

Critical (corroborated by all 3 reviewers, verified by hand-tracing
the control flow myself): the per-stream trailing-pop fix from the
previous commit only inspects a single `processStreamResponse` call's
own output. The MAX_TOKENS *recovery* loop explicitly proceeds only
when the truncated turn has NO functionCall yet -- exactly the
precondition under which the per-stream check's `hasToolCall` is
false and never fires. If the recovery continuation then calls a
tool (an ordinary agentic-loop event), `coalesceRecoveryPairs` merges
the two attempts via `appendRecoveryContinuationParts`, whose dedup
anchor is blind to `thought` parts -- reintroducing the exact
permanent-wedge hazard the previous fix targeted, just via the
cross-request merge path instead of a single stream. Fixed by
re-running the same trailing-only check on the truncated turn's own
parts immediately before the merge, using "does the continuation
introduce a functionCall" as the `hasToolCall` signal.

A second reviewer-proposed fix (broadening the trailing-only check to
scan the whole parts array, to also catch an unsigned episode
immediately preceding a functionCall within a single stream) was
implemented, then REVERTED after the full test suite caught a real
regression: DeepSeek legitimately emits unsigned thinking blocks
right before a functionCall as its normal, complete wire shape
(DeepSeek doesn't validate thinking signatures the way Anthropic
does). A whole-array scan can't distinguish "truncated mid-episode"
from "a provider that doesn't sign its thinking" -- only the trailing
position can, since a stream's own truncation can only ever leave the
dangling episode trailing (anything that followed it in the same
stream would already have flushed it). Kept the check trailing-only
and added a test pinning this as accepted residual risk, matching the
code's own pre-existing "Known limitation" note on wire-protocol
non-compliance.

Lower-priority structural fix (found by one reviewer, verified by
reading the code myself): `dropEmptyTextThinkingBlocks` computes "the
latest assistant message" once, before `stripTrailingAssistantPrefill`
can later pop a genuinely-empty trailing message and promote an
earlier one to "new latest" -- stale index. Verified the trigger
conditions overlap in practice (`stripTrailingAssistantPrefill` is
gated on model version 4.6+; `ensureLeadingAssistantThinking` is
gated on manual/explicit-budget mode; both are true simultaneously
for exactly the "4.6+ model with an explicit budget_tokens override"
configuration this PR's own escape-hatch targets). Fixed by
reordering the pipeline to run `stripTrailingAssistantPrefill` before
`dropEmptyTextThinkingBlocks`, preserving `mergeConsecutiveUserMessages`'s
existing adjacency to `dropEmptyTextThinkingBlocks` so its own
cleanup invariant (fixing up newly-adjacent user messages after an
assistant message is dropped) is unaffected.

Every fix and every reverted attempt was verified against the full
`geminiChat.test.ts` + `converter.test.ts` + `anthropicContentGenerator.test.ts`
suites (540 tests, only the one pre-existing unrelated User-Agent
failure) and against targeted mutation testing: each new regression
test was confirmed to fail when its guarded code path is disabled or
reverted, and to pass once restored.
netbrah pushed a commit to netbrah/qwen-code-upstream-pr that referenced this pull request Aug 20, 2026
Addresses a Critical from PR QwenLM#8260 review (and the related "three
uncoordinated predicates" Suggestion it escalated): contentText's
filter (`part.text && !part.thought`) and the XML-recovery removal
loop's filter (`isValidNonThoughtTextPart`, which additionally rejects
any part carrying `thoughtSignature`) disagreed on what counts as
"visible text." A part with `thoughtSignature` set but no `thought:
true` -- a real wire shape (loggingContentGenerator.ts's stream
aggregation spreads `thought` and `thoughtSignature` independently) --
was picked up by contentText for XML detection but survived the
removal loop untouched: recovery fired, but the raw `<invoke>` XML was
never stripped, leaking it into durable history duplicated alongside
the recovered functionCall.

Introduce a single `isVisibleTextPart` predicate (`Boolean(part.text)
&& !part.thought`) shared by contentText's initial computation, its
post-recovery recompute, and the removal loop's textIndices scan.
Deliberately the looser of the two prior predicates, not the stricter
one: narrowing contentText itself to exclude thoughtSignature-bearing
text would make `hasAnyContent` treat genuine visible text as absent,
throwing "Model stream ended with empty response text" on ordinary
turns. flushThoughtEpisode always sets `thought: true` on episode
parts, so `!part.thought` alone (already contentText's semantics)
already protects reasoning episodes from the removal loop without
isValidNonThoughtTextPart's stricter signature exclusion.

Adds a regression test that reproduces the leak on unfixed code
(confirmed failing before this fix, passing after) with a plain-text
part carrying a stray thoughtSignature and XML content.

Also addresses two outstanding test-coverage Suggestions from the same
review round:
- converter.test.ts: a multi-thinking-run case for
  ensureLeadingAssistantThinking, guarding the "only the first run
  moves" invariant against a hoist-all-thinking mutant that the
  existing single-run test couldn't catch.
- anthropicContentGenerator.test.ts: a generator-level adaptive-mode
  test mirroring the manual-mode one, guarding the `thinking?.type ===
  'enabled'` gate against a `!!thinking` regression that would
  reintroduce the hoist-every-thinking corruption on adaptive models.
- geminiChat.test.ts: asserts the interleaved-episode test's recorded
  JSONL turn (not just in-memory history) preserves both reasoning
  episodes and their signatures, guarding --resume fidelity against a
  recording-only regression that in-memory assertions can't see.
netbrah pushed a commit to netbrah/qwen-code-upstream-pr that referenced this pull request Aug 20, 2026
…t the latest

Addresses the review round on QwenLM#8260.

ensureLeadingThinkingOnLatestAssistantMessage repaired only the most
recent assistant message, which was wrong in two independent ways:

- QwenLM#3786 describes the anthropic-compatible rejection against a PRIOR
  assistant turn carrying tool_use, and injectEmptyThinkingOnToolUseTurns
  correspondingly repairs every tool_use turn. Under latest-only scoping a
  turn normalized while it was current reverts to the text-leading shape on
  the next request, so the failure surfaces one turn after the turn that
  produced it.
- Keying the reorder on "is this the latest assistant message" made a
  turn's serialization depend on its position in history, so the same turn
  went out two different ways on consecutive requests. Since
  addCacheControlToMessages anchors its breakpoint on the last user
  message, that rewrote the cached prefix and forced a full prompt-cache
  re-read every turn.

Renamed to ensureLeadingThinkingOnToolUseAssistantMessages and gated on
tool_use, matching the option's own documented scope.

Also in this round:

- Apply dropDanglingUnsignedTrailingThought inside the XML tool-call
  recovery branch, before the recovered functionCall parts are appended.
  Recovery's gate requires hasToolCall === false, which is exactly when the
  per-stream drop early-returns, so an unsigned trailing episode survived
  and was then paired with a tool_use -- permanently wedging the session
  once the tool result returned.
- Drop the dead `.filter(part => part !== null)` in the recording path;
  redactStructuredOutputArgsForRecording only returns null for parts the
  enclosing ternary already excludes.
- Correct dropDanglingUnsignedTrailingThought's doc, which overclaimed that
  trailing-only scope distinguishes a truncated signing-provider episode
  from a non-signing provider's ordinary trailing thought. It does not; a
  truncated DeepSeek stream has the identical shape. Document the accepted
  false positive and the fact that the coalescing call site never reaches
  the JSONL record.
- Document the mirror-image episode limitation: two adjacent text-less
  signed thought parts concatenate their signatures into one part valid for
  neither block, newly reachable on the OpenAI Responses wire (QwenLM#8169).

Tests: all four new behaviors are mutation-verified (fix reverted -> red,
restored -> green). 556/556 pass across geminiChat.test.ts and
anthropicContentGenerator/.
netbrah pushed a commit to netbrah/qwen-code-upstream-pr that referenced this pull request Aug 20, 2026
…gText is re-inserted

Addresses review round 3 finding R3-1 on QwenLM#8260. The reviewer is right and
this is a hole in the previous round's own fix.

The third dropDanglingUnsignedTrailingThought call site was placed after
`recovery.remainingText` was spliced back into consolidatedHistoryParts.
When the dangling unsigned episode PRECEDES the consumed XML text part, the
re-inserted text lands behind the episode, so the trailing-only check sees a
text part last, no-ops, and the appended functionCall parts persist
`[thought(unsigned), text, functionCall]` -- an active tool-use turn holding
unsigned thinking, which makes
dropUnsignedThinkingFromAssistantMessages throw on every subsequent request.
Same permanent wedge the call site was added to prevent, reached by a
different shape.

The previous round only considered the episode-trailing case and treated a
preceding unsigned episode as the documented non-trailing residual risk.
That was wrong here: the episode IS trailing at the moment the consumed text
parts are spliced out, and only the re-insertion pushes it out of last
position.

Moved the drop into that window -- after the splice-out, before both the
re-insertion and the append -- which is the only point where a dangling
episode is guaranteed to be the last element. `insertAt` is now clamped
against the post-drop length, since the drop can shorten the array.

Regression test uses the trigger shape the reviewer named: an unsigned
episode followed by a plain-text part carrying a stray thoughtSignature and
no thought flag (the wire shape isVisibleTextPart's own doc calls out as
real), with non-empty remainingText so the ordering is observable. Confirmed
failing before this change and passing after.

575/575 across geminiChat.test.ts and anthropicContentGenerator/ on top of
the merged main; tsc, eslint and prettier clean.
netbrah pushed a commit to netbrah/qwen-code-upstream-pr that referenced this pull request Aug 20, 2026
…ivergence verdict

Companion documentation to QwenLM#8533 and follow-up from PR QwenLM#8260's
architectural review round. Two docs:

- 2026-08-04-reasoning-episode-invariants.md: enumerates every place
  Content[]/Part[] history is mutated after a content generator
  produces it, and whether each site preserves the thought/
  thoughtSignature reasoning-replay invariant Anthropic's strict
  tool-use-chain contract depends on.
- 2026-08-04-resume-jsonl-reasoning-divergence.md: confirms --resume
  can reconstruct the exact dangling-unsigned-thought hazard PR QwenLM#8260
  fixed for the live path, because the in-memory fix in
  coalesceRecoveryPairs never reaches the on-disk JSONL transcript.

Produced by four independent reviewers on different model families,
cross-checked against each other and against the actual code.
@wenshao

wenshao commented Aug 23, 2026

Copy link
Copy Markdown
Collaborator

Maintainer verification — built a real local harness and ran this end-to-end

@netbrah @qqqys — I verified this against a real qwen CLI process talking to a mock Anthropic Messages endpoint (SSE), rather than at unit level, so the thing being asserted is the actual bytes the CLI puts back on the wire on the next turn. Verified at head 6b3e68adc5, A/B against the merge-base ac78acd3c5.

Verdict: the fix does what it says, and I could not find a regression. Recommending merge once it is brought up to date with main (it is 311 commits behind — see Merge readiness below, where I already validated the merged tree).

Harness — how the numbers below were produced

Two worktrees (ac78acd3c5 and 6b3e68adc5), each with its own npm ci + npm run bundle, run against a Node SSE server that speaks the Anthropic Messages protocol and logs every inbound request body:

ANTHROPIC_API_KEY=… ANTHROPIC_BASE_URL=http://127.0.0.1:PORT \
  node <tree>/dist/cli.js --approval-mode yolo --model <model> -p "Read alpha.txt and beta.txt"

HOME is isolated per run so no real ~/.qwen settings leak in. The server scripts turn 1 as a thinking/tool-call turn and turn 2 as a plain answer; the CLI really executes the read_file calls, and request #2 is the artifact under test — that is history after consolidation, converted back to the Anthropic wire format.


1. The primary bug reproduces, and the fix holds

interleaved + back-to-back

One correction to how #8258 frames the impact. The issue says this "degrades gracefully (content dropped, not corrupted)". On the wire it is worse than that. main does not just drop SIG-EPISODE-TWO-BBBB — it emits a single thinking block whose thinking field is the concatenation of both episodes' text while still carrying episode one's signature. An Anthropic thinking signature is computed over that block's own text, so what main sends is a mis-signed block, not a merely lossy one. That moves the failure mode from "model loses replay context" toward "server-side signature verification has something to reject", which I think strengthens the case for merging rather than deferring.

2. Secondary behaviours, including the regression I most wanted to disprove

split signature, ordering, manual mode

Scenario 5 is the one I went looking for a regression in. Removing the "hoist thinking to parts[0]" behaviour could plausibly break manual-mode extended thinking's leading-thinking requirement. It does not: with thinking:{type:'enabled'}, the new ensureLeadingThinkingOnToolUseAssistantMessages reproduces main's output byte-for-byte, including for the awkward [text, tool_use, thinking, tool_use] shape raised in the still-open thread on converter.test.ts:1177. That mechanism is real — the run is relocated even when it begins after a tool_use — but it is not a change in behaviour, and it is confined to manual mode. Adaptive mode is left in true chronological order (Scenario 4).

I also confirmed on the wire that interleaved-thinking-2025-05-14 is sent for both {type:'adaptive'} and {type:'enabled'}, so the premise behind the mergeConsecutiveAssistantMessages change is factually correct.

3. --resume fidelity

The session JSONL matches in-memory history exactly:

// PR — .qwen/projects/…/chats/<id>.jsonl
{"role":"model","parts":[
  {"text":"EPISODE-ONE: …","thought":true,"thoughtSignature":"SIG-EPISODE-ONE-AAAA"},
  {"functionCall":{"id":"toolu_alpha","name":"read_file",}},
  {"text":"EPISODE-TWO: …","thought":true,"thoughtSignature":"SIG-EPISODE-TWO-BBBB"},
  {"functionCall":{"id":"toolu_beta","name":"read_file",}}]}

// main — second signature gone, both texts under the first signature
{"role":"model","parts":[
  {"text":"EPISODE-ONE: …EPISODE-TWO: …","thought":true,"thoughtSignature":"SIG-EPISODE-ONE-AAAA"},
  {"functionCall":{"id":"toolu_alpha",}},{"functionCall":{"id":"toolu_beta",}}]}

Prompt-cache prefix stability also checks out: assistant turn 1 serializes identically across consecutive requests in a 3-turn manual-mode session. The only byte that moves between request #2 and #3 is the cache_control breakpoint relocating to the new last user message — identical on main.

4. Tests, mutation probes, merge readiness

tests and mutation probes

I ran three mutation probes rather than trusting a green suite, because a passing E2E proves nothing if the harness cannot see the behaviour. Each mutation was applied to the PR source, rebundled, and re-run through the same real-CLI path; all three produce visibly wrong wire output, so the new code is load-bearing and the harness is non-vacuous.

Probe 1 is worth calling out: reverting the XML-recovery removal predicate to the pre-PR .text !== undefined makes the reasoning episode vanish entirely — text and signature — from the turn-2 request. That confirms the #8003 interaction described in the PR body is real. Note it is a hazard this PR's own restructuring creates (pre-PR, episodes never lived in consolidatedHistoryParts, so the bare check could not reach them) and then closes in the same diff. A/B against main shows no difference there, which is the correct outcome.

Merge readiness. The branch is 311 commits behind main, and main has touched geminiChat.ts six times since the merge-base. I merged main (ea872a4621) into the branch locally: no conflicts, 585/585 tests green on the merged tree, and Scenario 1 still passes E2E there. So the staleness is real but benign.


Residual items — none blocking, listed for the record

  1. dropDanglingUnsignedTrailingThought's accepted false positive is real. A non-signing provider (DeepSeek) truncated mid-reasoning after a tool call loses its trailing reasoning from both history and the JSONL. The PR documents this and argues losing a fragment beats wedging a session, which I agree with. I did not reproduce it live — it needs a DeepSeek-shaped base URL — so this is code-reading only.
  2. Media parts now persist to the session JSONL. Declared in the PR body, and correct for --resume fidelity, but it does mean model-produced base64 inlineData lands on disk where it previously never did. Worth watching for image-producing models. Not exercised by my harness.
  3. Small doc-accuracy nit. The PR body says recordAssistantTurn "now records every consolidated part verbatim". Not quite: redactStructuredOutputArgsForRecording returns { functionCall } without spreading, so siblings on a functionCall part are dropped. In practice unreachable here — loggingContentGenerator.ts:1015 already builds { functionCall: part.functionCall } upstream — so this is wording, not behaviour.
  4. Stale test count. The body's "369 across both files" is now 571 across the three touched test files (585 on the merged tree). Worth refreshing before merge.
  5. 26 review threads are still open, but I read through them and none is a standing Critical at 6b3e68adc5: the two round-4 Criticals and the round-6 transport-continuation Critical are answered by the third and fourth dropDanglingUnsignedTrailingThought call sites, and round 7 raised Suggestions only.

Not covered by this verification

The real Anthropic API (mock only), the OpenAI Responses wire (#8169), and the DeepSeek provider path. Documented limitation 2 — two text-less signed thought parts concatenating into a signature valid for neither block — is unreachable on the Anthropic wire and so was not exercised here; it stays a genuine hazard for #8169 and is correctly flagged there.

中文版本

维护者验证 —— 搭了一套真实的本地环境做端到端验证

@netbrah @qqqys 我没有停留在单测层面,而是让真实的 qwen CLI 进程去访问一个模拟的 Anthropic Messages 端点(SSE),这样被断言的对象就是 CLI 在下一轮真正发到线上的字节。验证基于 head 6b3e68adc5,并与 merge-base ac78acd3c5 做 A/B 对照。

结论:这个修复确实做到了它声称的事情,我没有找到回归。建议合并,前提是先跟 main 同步(目前落后 311 个提交 —— 见下文"可合并性",我已经验证过合并后的结果)。

验证环境

两个 worktree(ac78acd3c56b3e68adc5),各自独立 npm ci + npm run bundle,对接一个用 Node 写的、说 Anthropic Messages 协议的 SSE 服务器,它会记录每一个进来的请求体:

ANTHROPIC_API_KEY=… ANTHROPIC_BASE_URL=http://127.0.0.1:PORT \
  node <tree>/dist/cli.js --approval-mode yolo --model <model> -p "Read alpha.txt and beta.txt"

每次运行都隔离 HOME,避免真实的 ~/.qwen 配置串入。服务器把第 1 轮编排成"思考 + 工具调用"的轮次,第 2 轮返回纯文本答复;CLI 会真正执行 read_file 调用,而第 2 个请求就是被测对象 —— 也就是整合之后的历史,再转换回 Anthropic 线格式的样子。

1. 主问题可复现,修复成立

见上方第一张截图。

#8258 描述的一处修正。 Issue 里说这是"优雅降级(内容丢失,而非损坏)"。在线格式上,情况比这更糟。main 不只是丢掉了 SIG-EPISODE-TWO-BBBB —— 它发出的是一个 thinking 区块,其 thinking 字段是两个片段文本的拼接,却仍然带着第一个片段的签名。Anthropic 的 thinking 签名是针对该区块自身文本计算的,所以 main 发出去的是一个签名与内容不匹配的区块,而不仅仅是有损的区块。这把失效模式从"模型丢失了可重放的推理上下文"推向了"服务端签名校验有理由拒绝该请求",我认为这反而更支持尽快合并,而不是继续搁置。

2. 次要行为,以及我最想证伪的那个回归

见上方第二张截图。

场景 5 是我专门去找回归的地方。去掉"把 thinking 提到 parts[0]"这个行为,理论上可能破坏手动模式扩展思考的"必须以 thinking 开头"的约束。结果并没有:在 thinking:{type:'enabled'} 下,新增的 ensureLeadingThinkingOnToolUseAssistantMessagesmain 的输出逐字节一致,包括 converter.test.ts:1177 那条尚未解决的评论所指出的 [text, tool_use, thinking, tool_use] 这种别扭形态。那条评论指出的机制是真实存在的 —— 即使 thinking 段起始于某个 tool_use 之后,它确实会被搬到最前面 —— 但这并不是行为变化,而且只发生在手动模式内。自适应模式保持了真正的时间顺序(场景 4)。

我还在线格式上确认了:interleaved-thinking-2025-05-14{type:'adaptive'}{type:'enabled'} 两种情况下都会发送,所以 mergeConsecutiveAssistantMessages 那处改动所依据的前提是成立的。

3. --resume 保真度

会话 JSONL 与内存中的历史完全一致:PR 分支上两个片段各自带着自己的签名落盘;main 上第二个签名消失,两段文本被并到第一个签名之下。

prompt cache 前缀的稳定性也没问题:在一个 3 轮的手动模式会话里,第 1 个 assistant 轮次在相邻两次请求中的序列化结果完全相同。请求 #2#3 之间唯一移动的字节是 cache_control 断点挪到了新的最后一条 user 消息上 —— 这一点在 main 上表现一致。

4. 测试、变异探针与可合并性

见上方第三张截图。

我没有满足于"测试全绿",而是跑了三个变异探针 —— 因为如果验证环境根本看不见这个行为,那么 E2E 通过什么也说明不了。每个变异都作用在 PR 源码上,重新打包,再走同一条真实 CLI 路径;三个变异都产生了肉眼可见的错误线格式输出,说明新增代码确实在承担作用,验证环境也不是空转的。

探针 1 值得单独说明:把 XML 恢复路径的判断谓词退回到修复前的 .text !== undefined,会让推理片段从第 2 个请求中彻底消失 —— 文本和签名一起没了。这印证了 PR 描述中提到的 #8003 交互问题是真实的。需要说明的是,这个隐患其实是本 PR 自身的结构调整所引入的(修复前推理片段根本不在 consolidatedHistoryParts 里,那个宽松判断够不着它们),并在同一个 diff 里被关掉。与 main 做 A/B 时这里没有差异,这正是应有的结果。

可合并性。 分支落后 main 311 个提交,而 main 自 merge-base 以来已经改过 geminiChat.ts 六次。我在本地把 mainea872a4621)合入了该分支:没有冲突,合并后的树上 585/585 测试全绿,场景 1 的 E2E 在合并后依然通过。所以落后是事实,但是良性的。

遗留事项 —— 都不阻塞合并,仅作记录

  1. dropDanglingUnsignedTrailingThought 所接受的误判是真实存在的。 一个不做签名的供应商(DeepSeek)如果在工具调用之后、推理中途被截断,其尾部推理会同时从历史和 JSONL 中丢失。PR 已经记录了这一点,并论证"丢一个片段好过把会话彻底卡死",我认同这个取舍。我没有实地复现它 —— 那需要一个 DeepSeek 形态的 base URL —— 所以这一条仅基于代码阅读。
  2. 媒体部件现在会写入会话 JSONL。 PR 描述中已声明,对 --resume 保真度而言也是正确的取舍,但这确实意味着模型产出的 base64 inlineData 会落到磁盘上,而此前从不会。对会产图的模型需要留意。我的验证环境没有覆盖这一条。
  3. 一处措辞上的小问题。 PR 描述说 recordAssistantTurn "现在会逐字记录所有整合后的部件"。并不完全准确:redactStructuredOutputArgsForRecording 返回的是 { functionCall },没有展开原部件,因此 functionCall 部件上的同级字段会被丢掉。不过在当前代码里实际不可达 —— loggingContentGenerator.ts:1015 在上游就已经构造了 { functionCall: part.functionCall } —— 所以这是措辞问题,不是行为问题。
  4. 测试数字已过期。 描述里的"两个文件合计 369 个"现在是三个被改测试文件合计 571 个(合并后的树上是 585)。建议合并前更新一下。
  5. 仍有 26 条评论线程未解决,但我逐条读过,在 6b3e68adc5没有仍然成立的 Critical:第 4 轮的两个 Critical 和第 6 轮的 transport-continuation Critical,都已由第三、第四个 dropDanglingUnsignedTrailingThought 调用点回应;第 7 轮只提出了 Suggestion。

本次验证未覆盖的部分

真实的 Anthropic API(本次仅用 mock)、OpenAI Responses 链路(#8169),以及 DeepSeek 供应商路径。已记录的限制 2 —— 两个无文本的已签名 thought 部件拼接出一个对两个区块都无效的签名 —— 在 Anthropic 链路上不可达,因此本次没有触发;它对 #8169 仍是真实隐患,PR 中已正确地标了出来。

wenshao
wenshao previously approved these changes Aug 23, 2026
@wenshao

wenshao commented Aug 23, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 23, 2026

Copy link
Copy Markdown
Collaborator

Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run

Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check.

Scripted assertions: 45 passed · 0 failed · 45 total

Flakiness gate: ✅ 3 changed test file(s) x 5 identical rounds, no divergence

中文 — 判定:✅ 通过 · 可合入(agent 判定)

沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码(与 base 构建 A/B 对照、无 mock harness 断言、定向门禁)。仅作为评审证据,不构成评审、批准或 CI 检查

脚本断言:45 通过 · 0 失败 · 45 总计

抖动门:✅ 3 changed test file(s) x 5 identical rounds, no divergence

Verification report

PR #8260 Deep Verification — fix(core): preserve every reasoning episode's signature during history consolidation

Verdict: merge-ready — 45/45 scripted assertions passed (pass: 45, fail: 0). Verified head: 6b3e68adc53b4b7a4b367e12cef76dae7cbe3a87, verified against merge-base tip 7385b278b2017a0b6bfeff59380d23b57848fd4a (HEAD^1 of the merge-ref checkout).

中文摘要
  • 结论merge-ready。45/45 脚本化断言通过,0 失败;未发现阻塞性问题。
  • A/B 结论(见「Central claim — A/B cell table」):用同一个多片段推理流驱动真实 GeminiChat,base 侧产生合并后的单一 thought 块(只保留第一个签名、位置被提前、截断场景下把 sig1 错挂在合并文本上、JSONL 丢失媒体部件);head 侧每个推理片段保留各自的签名与原始位置,截断产生的未签名尾片段被丢弃,XML 恢复与媒体记录均按声明工作。两臂各自 11/11 通过(base 臂断言的就是预期的"坏"形状)。
  • 转换器(见「Converter cell table」):手动思考模式下 text 开头的 tool_use 轮次被修复为 thinking 开头;自适应模式保持时间序;相邻 assistant 合并由"全部 thinking 提前"改为按序拼接;stripTrailingAssistantPrefill 重排序保住了被提升为最新轮次的已签名空文本 thinking 块;序列化与位置无关(prompt-cache 前缀稳定)。
  • 变异矩阵:6/6 变异体被 PR 自带测试击杀,零存活;中心新测试经"撤掉核心 hunk"验证非空泛(失败信息为行为断言而非崩溃)。
  • Findings:无阻塞项。仅一处更正(测试计划中的 369 个测试数字已过时,现为 465,命令本身照跑全绿)。
  • 未覆盖:逐 commit 归因(depth-2 浅克隆)、全仓测试/类型检查(由 PR 自身 CI 覆盖)、对真实 Anthropic/OpenAI 端点的联线验证、feat(core): add OpenAI Responses API content generator #8169 的 OpenAI Responses 转换器(不在此 base 中)。详见 "Not covered"。

Central claim — A/B cell table

Central claim: geminiChat.ts turn consolidation preserves every reasoning episode as its own Part, each with its own thoughtSignature, in its original position relative to tool calls — instead of merging all thought parts into one hoisted blob and keeping only the first signature.

Harness: harness/ab-harness.mjs drives the real GeminiChat.sendMessageStream (tsx over the TS source, zero module mocks — only injected collaborators: a fake ContentGenerator, a fake ChatRecordingService, a plain config object). Identical scripted streams on both arms; the base arm asserts the broken shapes are produced (an expected base failure is encoded as a passing assertion). Witnesses: 01-ab-head-fixed-shapes.png (head) and 02-ab-base-broken-shapes.png (base); raw logs harness/head-run.log, harness/base-run.log.

# scenario (stream shape) BASE cell (broken, asserted) HEAD cell (fixed, asserted)
S1 two episodes interleaved with two tool calls [{text:"AB",thought,sigA}, call1, call2] — episodes merged, hoisted, sigB lost (in-memory and JSONL) [{A,thought,sigA}, call1, {B,thought,sigB}, call2] — both signatures, original positions
S2 one episode, signature split across two chunks thoughtSignature:"sigFrag1"truncated thoughtSignature:"sigFrag1sigFrag2" — concatenated
S3 back-to-back episodes, no intervening tool call [{text:"AB",thought,sigA}, {final}] [{A,sigA}, {B,sigB}, {final}]
S4 stream truncated (MAX_TOKENS) mid-episode-2 after a tool call [{text:"ep1ep2 partial",thought,sig1}, call1]sig1 (valid for ep1 only) silently attached to the merged ep1+ep2 text [{ep1,sig1}, call1] — dangling unsigned trailing episode dropped
S5 reasoning episode + XML tool-call recovery on the same turn episode survives (lives in thoughtContentPart, outside the removal loop) episode survives in place: [{planning,sig-survive}, fc(read_file)]isVisibleTextPart keeps it out of the removal loop
S6 plain-text part with stray thoughtSignature + XML (the leak shape) raw XML consumed, stray-signature part retained byte-identical to base — parity cell; the leak existed mid-PR-history and is closed in the final state
S7 declared change: media part in a model turn history carries inlineData, but recorded JSONL drops it ([{text}] only) recorded JSONL carries inlineData verbatim — the declared --resume fidelity change, verified deliberate

Counts: head 11/11, base 11/11 — every flip cell proves the change load-bearing; S5/S6/S7 show the two related fixes and the declared recording change behave as described.

Converter cell table (secondary claim)

Harness: harness/converter-harness.mjs drives the real AnthropicContentConverter.convertGeminiRequestToAnthropic with Gemini histories and asserts the exact emitted Anthropic block arrays. Witnesses: 03-converter-head.png, 04-converter-base.png; logs harness/converter-{head,base}.log.

# scenario BASE HEAD
C1 manual mode (ensureLeadingAssistantThinking), turn converts to [text, thinking, tool_use] ships the invalid text-leading shape as-is repaired to [thinking, text, tool_use]
C2 adaptive mode (option off), same turn chronological pass-through chronological pass-through (parity — adaptive untouched)
C3a adjacent assistant messages merged, adaptive hoist: [thinkingX, thinkingY, textA, tool_use] concat: [textA, thinkingX, thinkingY, tool_use]
C3b same merge, manual mode hoisted concat then first thinking run moved to front — required shape, same result here
C4 same turn serialized with/without a later turn following (head-only invariant) n/a (mechanism absent) byte-identical serialization — position-independent, prompt-cache prefix stable
C5 empty trailing assistant (prefill artifact) popped; earlier tool turn carries signed empty-text thinking pop happens AFTER dropEmptyTextThinkingBlocks → promoted latest turn loses its signed thinking('') block[tool_use] only (invalid in manual mode) pop first → signed empty-text thinking kept: [thinking('',sigEmpty), tool_use]
C6 turn with two thinking runs: [text, thinkF, tu1, thinkS, tu2], manual mode pass-through only the FIRST run moves: [thinkF, text, tu1, thinkS, tu2] — later runs untouched

Counts: head 8/8, base 7/7 (C4 skipped on base — the mechanism does not exist there).

Mutation matrix (vacuity of the PR's new tests)

Driver: harness/matrix-driver.mjs applies each mutant to a scratch worktree at HEAD, runs the target suite, asserts the expected test titles go red, restores the file. Witness: 05-mutation-matrix-6-of-6-killed.png; log harness/matrix-run.log.

mutant guard removed/reverted suite killed red tests (expected ⊆ observed)
M1 episode-split condition deleted geminiChat.test.ts ✅ 1/352 back-to-back split test
M2 dropDanglingUnsignedTrailingThought disabled geminiChat.test.ts ✅ 5/352 all four call-site tests + the accepted-false-positive pin
M3 removal-loop predicate → bare .text !== undefined geminiChat.test.ts ✅ 4/352 episode-preservation test + 3 interaction tests
M6 removal-loop predicate → stricter isValidNonThoughtTextPart geminiChat.test.ts ✅ 2/352 the XML-leak regression test + the non-trailing-episode test
M4 leading-thinking scoped to latest message only converter.test.ts ✅ 2/113 every-tool_use-turn test + position-independence test
M5 merge restored to hoist-all-thinking converter.test.ts ✅ 3/113 chronological-merge tests + multi-run test

Zero survivors. Notes:

  • Central-test vacuity check: M1 is the revert of the central hunk's boundary logic; the new back-to-back test fails it with a behavioral assertion — AssertionError: expected [ { text: 'AB', …(2) }, …(1) ] to deeply equal [ …(3) ] (mutant merges episodes A+B; test expects two distinct episodes). Not a crash, not an import break.
  • Same-file positive control: every mutant lands in the very file its killing suite imports (geminiChat.ts ↔ geminiChat.test.ts; converter.ts ↔ converter.test.ts); M1's single kill proves the chosen vitest command collects tests that execute the mutated file.
  • Layered guards adjudicated, not conflated: M1 kills only the back-to-back shape because the interleaved shape is protected by a different clause (the flush on non-thought parts). The two guards cover disjoint shapes, so neither row is a false survivor of the other; reverting them together was therefore unnecessary.
  • Reverse mutation: the documented residual limitations (two unsigned back-to-back episodes merge; two text-less signed parts concatenate signatures) are pinned by tests asserting current behavior (both green in the gate run); a scratch "fix" for either would turn those pins red by design, so no candidate-further-fix run was made.

Targeted gates

harness/gate-driver.mjs — witness 06-gates-green-586-tests.png, log harness/gate-run.log:

gate result
geminiChat.test.ts + converter.test.ts 465 passed, 0 failed
anthropicContentGenerator.test.ts 121 passed, 0 failed

Suite liveness (the gate can go red) is proven by the mutation matrix itself — six distinct mutants turned it red.

Corrections

  • The PR's Reviewer Test Plan says the two suites contain 369 tests; they now contain 465 (+96 added by later review rounds). The command in the plan was run verbatim and passes; only the count is stale. This is a correction to the description, not a request to change code.
  • The metadata snapshot's baseRefOid (ac78acd…) has drifted and is not present locally; verification used the merge-ref base tip HEAD^1 = 7385b27 per the CI checkout contract.

Findings

No blocking findings. Two observations, both confirmations rather than defects:

  1. Declared recording change verified in both directions (S7): base's recordAssistantTurn silently drops inlineData/fileData from the JSONL record; head records them verbatim. The PR's Risk & Scope section declares exactly this (model-produced base64 media now persists on disk for --resume fidelity) — the measurement matches the declaration.
  2. The DeepSeek false positive is real, declared, and pinned: disabling the dangling-drop (M2) turns the documents the accepted false positive… test red along with the four protective call sites — the trade-off (losing a trailing reasoning fragment on a non-signing provider vs. permanently wedging a signing one) is encoded in tests, not just prose.

No injection-style instructions were present in the PR text.

Not covered

  • Per-commit attribution: the checkout is depth-2; only the aggregate HEAD^1..HEAD diff was verified. The snapshot lists 15 commits but git rev-list HEAD^1..HEAD^2 yields 1 locally (shallow boundary), so no per-commit table is presented.
  • Repo-wide suite, lint, typecheck: not re-run — the PR's own CI covers them; only the three affected test files were executed here.
  • Live-wire validation: no Anthropic/OpenAI credentials exist in this sandbox, so the converter oracle is the emitted message/block shape, not API acceptance. The manual-mode "text-leading is rejected" contract is verified against the converter's output shape only.
  • xml-tool-call-fallback.ts internals: unchanged by this PR; exercised only through the changed call sites.
  • OpenAI Responses wire (feat(core): add OpenAI Responses API content generator #8169): not present at this base. The Responses-shaped episode claim is covered only by the suite's Gemini-Part-shaped fixture (test at line 3677), not the actual feat(core): add OpenAI Responses API content generator #8169 converter.
  • Performance/ladder probes: not applicable — the change is a linear single-pass walk with string concatenation and adds no regex/scanner over untrusted text.
  • First verification round: no previous-report.md present; nothing carried forward.

Methodology

CI verify container (node:22-bookworm, node v22.23.2), merge-ref checkout at depth 2; npm ci + npm run build completed before this round. The A/B and converter harnesses import the real production TS modules directly with tsx — no module mocking anywhere in the unit-under-test path; collaborators enter only through constructor/config seams. The base control ran in a scratch worktree at HEAD^1 resolving external deps through the root node_modules (PR touches no package.json/lockfile); packages/core/node_modules was symlinked in (external packages only) and the code-under-test closure was grep-verified to contain zero @qwen-code/* imports, so the head tree's workspace symlinks could not contaminate the control. Mutations ran in a second scratch worktree with the head tree's built dist/ copied in to satisfy vitest's build-prerequisite guard (the three test files import nothing through the package entry, verified by grep; M1's kill additionally proves mutants take effect through src/). Both worktrees were removed after the A/B cells and matrix were captured. Raw per-arm logs and all harness scripts live in tmp/pr8260-verify-20260823-191426/harness/; evidence images in evidence/.

Flakiness gate log

rounds=5 files=3 skipped=0
file packages/core/src/core/anthropicContentGenerator/anthropicContentGenerator.test.ts: (cd packages/core) npx --no-install vitest run ./src/core/anthropicContentGenerator/anthropicContentGenerator.test.ts
file packages/core/src/core/anthropicContentGenerator/converter.test.ts: (cd packages/core) npx --no-install vitest run ./src/core/anthropicContentGenerator/converter.test.ts
file packages/core/src/core/geminiChat.test.ts: (cd packages/core) npx --no-install vitest run ./src/core/geminiChat.test.ts


per-file results (P=pass F=fail I=infra-exit, one letter per run):
  packages/core/src/core/anthropicContentGenerator/anthropicContentGenerator.test.ts: PPPPP
  packages/core/src/core/anthropicContentGenerator/converter.test.ts: PPPPP
  packages/core/src/core/geminiChat.test.ts: PPPPP

verdict: pass
summary: 3 changed test file(s) x 5 identical rounds, no divergence

--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/core/src/core/anthropicContentGenerator/anthropicContentGenerator.test.ts: P (exit 0)
round 1 · packages/core/src/core/anthropicContentGenerator/converter.test.ts: P (exit 0)
round 1 · packages/core/src/core/geminiChat.test.ts: P (exit 0)
round 2 · packages/core/src/core/anthropicContentGenerator/anthropicContentGenerator.test.ts: P (exit 0)
round 2 · packages/core/src/core/anthropicContentGenerator/converter.test.ts: P (exit 0)
round 2 · packages/core/src/core/geminiChat.test.ts: P (exit 0)
round 3 · packages/core/src/core/anthropicContentGenerator/anthropicContentGenerator.test.ts: P (exit 0)
round 3 · packages/core/src/core/anthropicContentGenerator/converter.test.ts: P (exit 0)
round 3 · packages/core/src/core/geminiChat.test.ts: P (exit 0)
round 4 · packages/core/src/core/anthropicContentGenerator/anthropicContentGenerator.test.ts: P (exit 0)
round 4 · packages/core/src/core/anthropicContentGenerator/converter.test.ts: P (exit 0)
round 4 · packages/core/src/core/geminiChat.test.ts: P (exit 0)
round 5 · packages/core/src/core/anthropicContentGenerator/anthropicContentGenerator.test.ts: P (exit 0)
round 5 · packages/core/src/core/anthropicContentGenerator/converter.test.ts: P (exit 0)
round 5 · packages/core/src/core/geminiChat.test.ts: P (exit 0)

Evidence images

01-ab-head-fixed-shapes

02-ab-base-broken-shapes

03-converter-head

04-converter-base

05-mutation-matrix-6-of-6-killed

06-gates-green-586-tests

Harness scripts and raw logs are in the workflow run artifacts (7-day retention).

Qwen Code · sandboxed verification

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

⏸️ Deferring to @wenshao — the review is clean at 6b3e68adc5 (findings in the stage comments above), but the PR now changes 589 production lines in core, which crosses the two-tier rule's 500-line mark where the gate must hand the approval decision to a maintainer instead of casting the bot's vote. Your end-to-end verification and approval already stand on this commit; the remaining call is whether the second approval should be the bot's, a second human's, or an admin merge. The stale round-6 CHANGES_REQUESTED from this bot account (commit 2fe2ee32) still pins reviewDecision and can be dismissed once you pick a path, and the /verify report will land in the lifecycle comment. Needs a human call on this one.

@wenshao
wenshao disabled auto-merge August 23, 2026 19:10
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Triage re-run completed without a new review.

⚠️ The bot's only review on 6b3e68adc53b4b7a4b367e12cef76dae7cbe3a87 is a COMMENTED one, which carries no vote — so it has no verdict of its own on this commit, and main needs two approving reviews: an approval left by another account is a separate vote and does not count as the bot's own. Two different things look like this, and the stage-3 comment above says which: the triage skill deferring on purpose at 3/5 — a fork refactor hitting the approval guardrail, or a core change escalated for maintainer awareness, both normal outcomes — or an earlier approval that a push dismissed, leaving only the comment behind, which needs a fresh review.

⚠️ 机器人在 6b3e68adc53b4b7a4b367e12cef76dae7cbe3a87 上唯一的评审是 COMMENTED不带票 —— 因此它在该 commit 上没有自己的裁决,而 main 需要两个批准(其他账号的批准是另一张票)。有两种情况长这样,上方的 stage-3 评论会说明是哪一种:triage skill 在 3/5 时有意 defer(fork refactor 命中审批护栏,或核心改动被升级交由维护者把关,两者都是正常结果);或者更早的批准被一次推送作废、只剩下这条评论,此时需要重新评审。

The stage comments above were updated with the latest result. View workflow run.

上方各阶段评论已更新为最新结果。查看工作流运行

Palanisamy, Dinesh and others added 2 commits August 25, 2026 22:52
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Downgraded from Approve to Comment: CI failing: fallback-comment, review-pr, ack-review-request, resolve-pr, delay-automatic-review, authorize, Post Coverage Comment, Integration Tests (CLI, No Sandbox), Desktop Shell (${{ matrix.os }}), Test (macos-latest, Node 22.x), Test (windows-latest, Node 22.x), precheck-pr / precheck. Partially reviewed — gaps disclosed.

Not explored to full depth (tool budget reached): "agent reverse-audit (round 5)": running the three tests under vitest to confirm-by-execution (npm install/build prerequisite not attempted at the ceiling); substituted a mechanical source trac….

Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.

Deferred under the convergence posture (round 8, not a blocker) — recorded, not requested in this round:

  • packages/core/src/core/geminiChat.test.ts:2924 — [review] D8-1: new test duplicates the chatWithRecorder helper inline
  • packages/core/src/core/geminiChat.test.ts:11078 — [probe] R7-3: fourth drop call site's signed-keep direction is pinned by no test
  • packages/core/src/core/geminiChat.ts:5832 — [review] R6-3: transport-continuation contentText recompute is a dead store
  • packages/core/src/core/geminiChat.ts:5614 — [review] D8-2: stale "Third call site" ordinal contradicts the doc-block numbering
  • packages/core/src/core/geminiChat.test.ts:4882 — [review] D8-3: "Unreachable on the Anthropic wire" contradicts the converter's signed-empty-text handling
  • packages/core/src/core/geminiChat.ts:5862 — [review] R6-4: record mapping drops sibling fields of functionCall parts (unreachable today)
  • packages/core/src/core/geminiChat.ts:998 — [review] R7-8: 'Return-value shape' doc still claims previousParts feedback and stream order the code does not uphold
中文说明

⚠️ 已从批准降级为评论:CI failing: fallback-comment, review-pr, ack-review-request, resolve-pr, delay-automatic-review, authorize, Post Coverage Comment, Integration Tests (CLI, No Sandbox), Desktop Shell (${{ matrix.os }}), Test (macos-latest, Node 22.x), Test (windows-latest, Node 22.x), precheck-pr / precheck。 仅完成部分审查,审查缺口已披露。

未探索到全部深度(达到工具调用预算):"agent reverse-audit (round 5)"running the three tests under vitest to confirm-by-execution (npm install/build prerequisite not attempted at the ceiling); substituted a mechanical source trac…

未审查:反向审计——在 5 轮的反审轮数上限内未收敛。

收敛姿态下延后(第 8 轮,非阻断)——已记录,本轮不要求修改:共 7 条(原文未翻译,列表见上方英文部分)。

— qwen3.8-max via Qwen Code /review (v0.22.0)

@netbrah

netbrah commented Aug 27, 2026

Copy link
Copy Markdown
Contributor Author

@qwen-code /triage

…t-consolidation

Resolve the geminiChat -> llm-chat split plus main's Gemini -> Llm
identifier rename:

- geminiChat.ts: take main's re-export shim and 3-way replay this
  branch's changes onto llm-chat.ts (git merge-file against the merge
  base). The one non-applying hunk was a doc comment both sides rewrote;
  keep this branch's text with main's {@link LlmChat...} target.
- anthropicContentGenerator/converter.ts: take main's doc paragraph — it
  already states this branch's point and uses the renamed
  convertAnthropicResponseToLlm link.
- converter.test.ts / anthropicContentGenerator.test.ts /
  llm-chat.test.ts: keep this branch's tests, retargeted to main's
  renamed API (convertLlmRequestToAnthropic, convertAnthropicResponseToLlm,
  LlmChat).

tsc -p packages/core clean; 699 tests pass.

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partially reviewed — gaps disclosed.

4 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:

  • F1 new quiet-completion test hand-rolls the chatWithRecorder helper — already reported (D8-1, round-8 deferral paragraph, review 5029220263)
  • F2 transport-continuation contentText recompute dead store — already reported (comment 3791620693, R6-3)
  • RA-R3-B transport-continuation drop hardcoded gate over-pops on STOP-finished turns — already reported (comment 3791620688, R7-2)
  • RA-R4-A 'Return-value shape' doc order claim vs the function's own hoist — already reported (comment 3791620699, R7-8)

Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI (fork-PR authorization gate) and its E2E suite did not run locally; the packages/core unit suite covering every changed file ran green in CI (624 files, 22566 tests) and locally during the review.

Not explored to full depth (tool budget reached): chunk 2: executing npx vitest run src/core/anthropicContentGenerator/converter.test.ts — the review worktree has no node_modules (startup error: Cannot find package…; chunk 7: executing the new/changed tests in this worktree — the vitest globalSetup guard requires packages/core/dist/index.js , and npm run build in packages/core e….

Not reviewed: reverse audit — an auditor ran and opened its brief, but no agent was launched with the prompt the CLI built — the launch was written by hand, and what the agent was actually asked is not what this skill certifies.

Test Plan (not a blocker): packages/core/src/core/geminiChat.test.tsno such file or directory.

Deferred under the convergence posture (round 9, not a blocker) — recorded, not requested in this round:

  • packages/core/src/core/anthropicContentGenerator/converter.ts:1465 (+3 locations) — [review] D9-1: three diff-added references point at the deprecated geminiChat.ts shim / pre-rename interface (converter.ts:1465, converter.ts:1743, llm-chat…
  • packages/core/src/core/llm-chat.test.ts:16312 — [probe] D9-2: multi-pair coalescing gate re-evaluation (the property that defeats the two-pair wedge) has no test witness
中文说明

仅完成部分审查,审查缺口已披露。

本轮确认的 4 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。

未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI (fork-PR authorization gate) and its E2E suite did not run locally; the packages/core unit suite covering every changed file ran green in CI (624 files, 22566 tests) and locally during the review。

未探索到全部深度(达到工具调用预算):chunk 2:executing npx vitest run src/core/anthropicContentGenerator/converter.test.ts — the review worktree has no node_modules (startup error: Cannot find package…;chunk 7:executing the new/changed tests in this worktree — the vitest globalSetup guard requires packages/core/dist/index.js , and npm run build in packages/core e…

未审查:反向审计——有审计 agent 运行并打开了自己的 brief,但没有 agent 是用 CLI 构建的 prompt 启动的——启动 prompt 是手写的,agent 实际被要求做的并不是本 skill 所认证的内容。

Test Plan(非阻断):packages/core/src/core/geminiChat.test.tsno such file or directory

收敛姿态下延后(第 9 轮,非阻断)——已记录,本轮不要求修改:共 2 条(原文未翻译,列表见上方英文部分)。

— qwen3.8-max via Qwen Code /review (v0.22.3)

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partially reviewed — gaps disclosed.

7 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:

  • R10-1 dead {@link} to ConvertGeminiRequestToAnthropicOptions at converter.ts:1465 — already reported (D9-1, round-9 deferral paragraph, review 5060008666)
  • R10-2 hand-built recorder LlmChat at llm-chat.test.ts:3019 — already reported (D8-1, round-8 deferral paragraph, review 5029220263)
  • R10-3 fourth drop site's literal-true over-pop at llm-chat.ts:5998 — already reported (comment 3791620688, R7-2)
  • R10-4 geminiChat.ts's flushThoughtEpisode dead reference at converter.ts:1743 — already reported (D9-1, round-9 deferral paragraph, review 5060008666)
  • R10-5 geminiChat.ts's recovery loop dead reference at llm-chat.test.ts:16264 — already reported (D9-1, round-9 deferral paragraph, review 5060008666)
  • R10-6 transport-continuation contentText dead store at llm-chat.ts:6023 — already reported (comment 3791620693, R6-3)
  • R10-7 stale "Third call site" ordinal at llm-chat.ts:5805 — already reported (D8-2, round-8 deferral paragraph, review 5029220263)

Not reviewed: reverse audit — reached the 5-round cap without two consecutive dry rounds (rounds 3, 4, 5 each reported findings already reported in earlier rounds; every chunk was audited in each round).

Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI (fork-PR authorization gate) and its E2E suite did not run locally.

Not explored to full depth (tool budget reached): chunk 9: none — but I did not fully trace whether the OpenAI Responses converter can actually emit two *adjacent* text-less signed reasoning parts in a real response (it…; "agent 1b": none — no check was cut short by the tool budget..

Test Plan (not a blocker): packages/core/src/core/geminiChat.test.tsno such file or directory.

Deferred under the convergence posture (round 10, not a blocker) — recorded, not requested in this round:

  • packages/core/src/core/llm-chat.ts:5607 — [probe] D10-1: comment cites responses-converter.ts, which does not exist in this commit

Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had no anchor this round could use either — none at all, one with no certifier, one certified by an identity other than the one this round runs under, or one this round's fetch refused or resolved to the head — so the next review re-reads the whole diff unless recovery grafts an earlier own anchor that the round running it can use onto the complete work list this round leaves behind, and keeps doing so until a round's marker carries an anchor again or a graft lands that the round running it can use. (Stated, not acted on — this changes nothing about what the round posts.)

中文说明

仅完成部分审查,审查缺口已披露。

本轮确认的 7 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。

未审查:reverse audit — reached the 5-round cap without two consecutive dry rounds (rounds 3, 4, 5 each reported findings already reported in earlier rounds; every chunk was audited in each round)。

未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI (fork-PR authorization gate) and its E2E suite did not run locally。

未探索到全部深度(达到工具调用预算):chunk 9:none — but I did not fully trace whether the OpenAI Responses converter can actually emit two *adjacent* text-less signed reasoning parts in a real response (it…"agent 1b"none — no check was cut short by the tool budget.

Test Plan(非阻断):packages/core/src/core/geminiChat.test.tsno such file or directory

收敛姿态下延后(第 10 轮,非阻断)——已记录,本轮不要求修改:共 1 条(原文未翻译,列表见上方英文部分)。

机制健康:本轮未能干净收尾,因而扣留了增量锚点,而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff,除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上;并会一直如此,直到某一轮的标记重新带上锚点,或落地的嫁接能被运行该轮的评审使用。(仅陈述,不据此行动——这不改变本轮发布的任何内容。)

— qwen3.8-max via Qwen Code /review (v0.22.3)

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partially reviewed — gaps disclosed.

4 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:

  • R11-1 hand-inlined chatWithRecorder construction — already reported (D8-1, round-8 deferral paragraph, review 5029220263)
  • R11-2 geminiChat.ts recovery-loop misattribution — already reported (D9-1, round-9 deferral paragraph, review 5060008666)
  • R11-3 transport-continuation contentText dead store — already reported (comment 3791620693, R6-3)
  • R11-4 redacted_thinking membership of isThinking unpinned — already reported (comment 3791620697, R7-6)

Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

Not reviewed: reverse audit — reached the 5-round cap without two consecutive dry rounds (rounds 2, 3, 4 and 5 each reported findings; every chunk was audited in each round).

Test Plan (not a blocker): packages/core/src/core/geminiChat.test.tsno such file or directory.

Deferred under the convergence posture (round 11, not a blocker) — recorded, not requested in this round:

  • packages/core/src/core/llm-chat.ts:6003 — [probe] D11-1: transport-continuation prefix insertion's ordering half is unpinned (splice only runs against an empty array in tests)
  • packages/core/src/core/llm-chat.test.ts:4466 — [review] D11-2: truncation test cites a flushThoughtEpisode 'Known limitation' note that does not exist
  • packages/core/src/core/llm-chat.test.ts:19237 — [probe] D11-3: remainingText re-insertion index (insertAt > 0) unpinned — head-insertion mutant stays green
  • packages/core/src/core/llm-chat.ts:5811 — [probe] D11-4: placement comment overclaims the drop-before-reinsertion boundary as load-bearing (measured inert)
  • packages/core/src/core/llm-chat.test.ts:16310 — [probe] D11-5: coalescing-site drop's thought-only restriction unpinned at that call site
  • packages/core/src/core/llm-chat.ts:5707 — [review] D11-6: 'Known limitation note above' pointer lands on notes documenting different failure modes
  • packages/core/src/core/anthropicContentGenerator/converter.test.ts:1319 — [probe] D11-7: test reachability rationale names a recovery-coalescing source all three continuation paths gate out of existence
  • packages/core/src/core/llm-chat.test.ts:16310 — [probe] D11-8: coalescing-site drop's multi-pair path (iterations i>=1) unpinned — i===0-gated mutant passes the whole suite

Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had no anchor this round could use either — none at all, one with no certifier, one certified by an identity other than the one this round runs under, or one this round's fetch refused or resolved to the head — so the next review re-reads the whole diff unless recovery grafts an earlier own anchor that the round running it can use onto the complete work list this round leaves behind, and keeps doing so until a round's marker carries an anchor again or a graft lands that the round running it can use. (Stated, not acted on — this changes nothing about what the round posts.)

中文说明

仅完成部分审查,审查缺口已披露。

本轮确认的 4 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。

未审查(原文为英文):build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

未审查(原文为英文):reverse audit — reached the 5-round cap without two consecutive dry rounds (rounds 2, 3, 4 and 5 each reported findings; every chunk was audited in each round).

Test Plan(非阻断):packages/core/src/core/geminiChat.test.tsno such file or directory

收敛姿态下延后(第 11 轮,非阻断)——已记录,本轮不要求修改:共 8 条(原文未翻译,列表见上方英文部分)。

机制健康:本轮未能干净收尾,因而扣留了增量锚点,而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff,除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上;并会一直如此,直到某一轮的标记重新带上锚点,或落地的嫁接能被运行该轮的评审使用。(仅陈述,不据此行动——这不改变本轮发布的任何内容。)

— qwen3.8-max via Qwen Code /review (v0.23.0)

@wenshao

wenshao commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator

Maintainer re-verification — rebuilt the real harness at the current head

@netbrah @qqqys — following up on my 2026-08-23 verification, which was done at 6b3e68adc5. Since then the branch absorbed main's geminiChat.tsllm-chat.ts rename plus four merges, and one new commit (14292a7342). That is exactly the situation where a fix quietly loses a limb, so I rebuilt the harness from scratch and re-ran everything at 19dcebfee6, A/B against the current merge-base 60161cb64a.

Verdict: the fix survives the rename intact, all previous conclusions still hold, and I found no regression. Still recommending merge. Two new findings below narrow the scope of two claims in the PR body — neither is a defect, and one of them makes the PR less risky than its own description suggests.

Harness — how every number below was produced

One worktree at 19dcebfee6 with its own npm ci. Two bundles from that same tree:

  • PR armnpm run bundle as-is.
  • main arm — the three production files (llm-chat.ts, anthropicContentGenerator/converter.ts, anthropicContentGenerator/anthropicContentGenerator.ts) replaced with git show 60161cb64a:<path>, re-bundled. Since the PR touches only those three, that arm is exactly main-at-the-merge-base. Verified per-bundle: the PR arm contains flushThoughtEpisode, the main arm contains thoughtContentPart, and neither contains the other.

Both arms run the real bundled CLI against a ~180-line Node server speaking the Anthropic Messages SSE protocol, which logs every inbound request body to JSONL:

ANTHROPIC_API_KEY=… ANTHROPIC_BASE_URL=http://127.0.0.1:PORT ANTHROPIC_MODEL=… \
  node <dist>/cli.js --approval-mode yolo --model <model> -p "Read alpha.txt and beta.txt"

Each run gets an isolated HOME (so no real ~/.qwen leaks in) and its own workspace. The CLI really executes the read_file calls, so the artifact under test is the assistant message inside wire request #2 — consolidated history, converted back to the Anthropic wire format — plus the session JSONL, which is what --resume rehydrates.

Model choice selects the thinking mode: claude-sonnet-4-5-*thinking:{type:'enabled',budget_tokens} (manual), claude-sonnet-4-6-*thinking:{type:'adaptive'}.


1. The fix still does what it says at the new head

signature preservation

All three re-confirm at 19dcebfee6. The middle panel is the new one: the scenario 14292a7342 added is reachable end-to-end. A tool-result continuation that answers with signed reasoning only — no text, no tool call — is retried by the no-progress loop until it is accepted (5 attempts in my run); on the PR both signed episodes land in the session JSONL, on main they collapse under SIG-QUIET-A.

I'll repeat the correction from last round because it is still the strongest argument for merging: on the wire, main does not drop the second signature, it emits a thinking block whose text is both episodes concatenated while still carrying the first episode's signature. That is a mis-signed block, not a lossy one.

2. Merge-damage check — the rename did not eat any of the fix

I diffed the PR's own added lines at the old head against the new head, rather than trusting a green suite:

  • llm-chat.ts343 added lines on each side; the set difference is 4 lines, all mechanical: GeminiChatLlmChat in one doc reference, and consolidatedHistoryPartsacceptedTurnParts in three places plus its new binding (main's quiet-completion placeholder). No behavioural line changed.
  • converter.ts — 3 doc lines left the delta because main independently added the same text.
  • anthropicContentGenerator.ts — 6 added lines, identical.

Suites on the merged tree: llm-chat.test.ts + converter.test.ts + anthropicContentGenerator.test.ts698 passed / 698. (The PR body still says 369; it was 571 at my last review. Worth refreshing before merge.)

3. Manual mode is still byte-identical; adaptive mode is where the wire actually changes

ordering and mode gating

Same conclusion as last round, re-measured. History now stores the true chronological order, and in manual mode ensureLeadingThinkingOnToolUseAssistantMessages puts the wire back to exactly what main sends. The one genuine wire change is adaptive mode, where text now precedes thinking. The PR's argument for that is sound (interleaved-thinking-2025-05-14 is sent unconditionally whenever thinking is set — I re-confirmed that on the wire for both enabled and adaptive), but it is the one behaviour a mock cannot bless. If anyone has a live Anthropic key, that single request is the thing worth smoke-testing before merge.

4. New: two claims in the PR body are narrower than they read — both in the PR's favour

reachability

(a) "Each episode is preserved in its original position" is true of the algorithm but not observable on the Anthropic wire. I scripted a stream that genuinely interleaves — thinking(E1) → tool_use(alpha) → thinking(E2) → tool_use(beta) — and on the stock PR build history comes out as [E1, E2, alpha, beta], not [E1, alpha, E2, beta]. The cause is upstream of this PR: anthropicContentGenerator.ts buffers every tool_use block into deferredToolCalls and flushes the whole batch at message_delta, so llm-chat.ts never sees a tool call between two episodes. That code is pre-existing and byte-identical at the merge-base. To prove the attribution I built a probe arm with one line changed (yield tool_use inline instead of deferring): the PR then produces true interleaving all the way to the wire and the JSONL, while main still collapses to one mis-signed block. So the algorithm is right, and the ordering sentence in the description is describing something the Anthropic wire cannot show today. Episode↔episode and episode↔text ordering do reach the wire (§3) — it is specifically the interleaving with tool calls that does not.

(b) dropDanglingUnsignedTrailingThought's accepted false positive is unreachable on this wire — and so is call site #1's protection. Last round I flagged this as code-reading only because it needs a DeepSeek-shaped endpoint. This time I went after it directly: scripted tool_use → thinking(no signature_delta) so the unsigned episode is genuinely last in the stream. The same deferredToolCalls batching means the functionCall is always the last part of a tool-use turn, so the episode can never be trailing there — the guard does not fire, and PR and main are identical. The OpenAI-compatible converter batches the same way (getCompletedToolCalls() runs only on finish_reason), so by inspection the same holds there; I did not run that wire. Net: the documented false positive costs nothing on either real wire today, and call site #1 is correspondingly inert there. Call sites #2#4 (XML recovery, recovery-coalescing, transport continuation) are unaffected and stay live.

5. Mutation probes — the harness is not vacuous

mutations and gates

Four mutations, each applied to PR source, re-bundled, re-run through the same real-CLI path; 4 / 4 killed. Two worth calling out:

Merge readiness

56 commits behind main (was 311). Main has not touched llm-chat.ts since the merge-base; one commit (#10896) touched the anthropic directory. git merge-tree against origin/main@e09a45c5 produces zero conflicts. PR CI at this head is green across Test, Lint & Static, Integration (no-AK), Desktop Shell and web-shell E2E.

reviewDecision is still CHANGES_REQUESTED, pinned by the bot's own review on 2fe2ee32b4 (2026-08-12) — six commits stale. That is the deadlock, not a standing objection: of the 26 unresolved review threads, every [Critical] is marked outdated and answered by a later commit, and everything current is a [Suggestion].

Residuals — unchanged, none blocking

  1. Media parts now persist to the session JSONL. Declared in the PR body and correct for --resume fidelity. Still not exercised here, and it cannot be on this wire: an Anthropic assistant message carries only text / thinking / tool_use, and the response path never builds an inlineData part. It stays a Gemini-wire consideration.
  2. Doc-accuracy nit (unchanged): the body says recordAssistantTurn "records every consolidated part verbatim". redactStructuredOutputArgsForRecording returns { functionCall } without spreading, so siblings on a functionCall part are dropped — unreachable in practice, so wording only.
  3. Stale test count in the body — 369 → 698.
  4. Two one-line description edits would help a future reader: the ordering claim in §4(a), and noting that limitation 如何自定义密钥文件 .env可能与其他文件冲突 #3's false positive is unreachable on the wires shipped today (§4(b)).

Not covered by this verification

The real Anthropic API (mock only — see §3 for the one request worth a live smoke test), the OpenAI Responses wire (#8169), and the DeepSeek provider path other than by inspection. Documented limitation #2 remains unreachable on the Anthropic wire and is correctly flagged for #8169.

中文说明

维护者复验 —— 在当前 head 上重建真实验证环境

@netbrah @qqqys 这是对我 2026-08-23 那次验证(基于 6b3e68adc5)的跟进。此后分支吸收了 main 的 geminiChat.tsllm-chat.ts 重命名和四次合并,外加一个新提交 14292a7342。这正是「修复被合并悄悄吃掉一半」的高发场景,所以我把验证环境整个重建了一遍,在 19dcebfee6 上重跑,并与当前 merge-base 60161cb64a 做 A/B。

结论:重命名没有伤到修复,上一轮的全部结论依然成立,没有发现回归。仍然建议合并。 下面两条新发现只是收窄了 PR 描述里两处说法的适用范围 —— 都不是缺陷,其中一条反而说明这个 PR 比它自己的描述更安全。

验证环境

一个 19dcebfee6 的 worktree,独立 npm ci。从同一棵树打出两个包:PR 臂直接 npm run bundlemain 臂把三个生产文件用 git show 60161cb64a:<path> 换回去再打包 —— 因为本 PR 只动这三个文件,这一臂就等于 merge-base 上的 main。逐包核对过:PR 臂含 flushThoughtEpisode,main 臂含 thoughtContentPart,互相都不含对方。

两臂都用打包后的真实 CLI 去访问一个约 180 行、说 Anthropic Messages SSE 协议的 Node 服务器,它把每个进来的请求体写成 JSONL 流水账。每次运行隔离 HOME 和工作区;CLI 会真正执行 read_file,所以被测对象是第 2 个请求里的 assistant 消息 —— 也就是整合后的历史再转回 Anthropic 线格式的样子 —— 外加会话 JSONL(--resume 读回的东西)。模型名选择思考模式:claude-sonnet-4-5-* 走手动模式,claude-sonnet-4-6-* 走自适应模式。

1. 在新 head 上修复依然成立(图 1)

三个场景都在 19dcebfee6 上复现。中间那块是新增的:14292a7342 新加的场景在真实链路上可达。一个只回签名推理、没有正文也没有工具调用的工具结果续写轮次,会被 no-progress 循环反复重试直到被接受(我这次跑到第 5 次尝试);PR 上两个带签名的片段都落进 JSONL,main 上它们被并到 SIG-QUIET-A 之下。

上一轮那个更正仍然是最有力的合并论据:线格式上 main 不是「丢掉」第二个签名,而是发出一个 thinking 区块,其文本是两段推理的拼接,却仍然带着第一个片段的签名 —— 这是签名与内容不匹配,而不只是有损。

2. 合并伤害检查 —— 重命名没有吃掉修复

我没有靠「测试全绿」,而是把 PR 自己的新增行在新旧两个 head 上做了集合比对:

  • llm-chat.ts —— 两边各 343 行新增,差集只有 4 行,全是机械改动:文档里 GeminiChatLlmChat,以及三处 consolidatedHistoryPartsacceptedTurnParts 加它的新绑定(main 引入的 quiet-completion 占位符)。没有任何行为行发生变化。
  • converter.ts —— 3 行注释离开了差集,因为 main 自己也加了同样的文字。
  • anthropicContentGenerator.ts —— 6 行新增,完全一致。

合并后的树上跑三个测试文件:698 / 698 全过。(PR 描述里还写着 369;上一轮我看到的是 571。建议合并前更新。)

3. 手动模式仍然逐字节一致;真正变化的是自适应模式(图 2)

结论与上一轮一致,本轮重新实测。历史现在按真实时间顺序存储,而手动模式下 ensureLeadingThinkingOnToolUseAssistantMessages 把线格式还原成与 main 完全相同的样子。唯一真实的线格式变化在自适应模式text 现在排在 thinking 前面。PR 给出的理由是成立的(只要设置了 thinkinginterleaved-thinking-2025-05-14 就会无条件发送 —— 我在 enabledadaptive 两种情况下都在线上重新确认过),但这恰恰是 mock 无法背书的一点。如果有人手头有真实的 Anthropic key,合并前值得拿这一个请求做一次冒烟。

4. 新发现:PR 描述里两处说法的适用范围比字面更窄 —— 而且都对 PR 有利(图 3)

(a)「每个片段保持在原始位置」对算法成立,但在 Anthropic 链路上观察不到。 我编排了一个真正交错的流:thinking(E1) → tool_use(alpha) → thinking(E2) → tool_use(beta),而 PR 的标准构建产出的历史是 [E1, E2, alpha, beta],不是 [E1, alpha, E2, beta]。原因在本 PR 之上游:anthropicContentGenerator.ts 把每个 tool_use 区块缓冲进 deferredToolCalls,等到 message_delta 才整批吐出,所以 llm-chat.ts 根本看不到夹在两个片段之间的工具调用。那段代码在 merge-base 上逐字节相同、本 PR 未作改动。为了坐实归因,我做了一个只改一行(把 tool_use 就地吐出而不缓冲)的探针构建:此时 PR 一路把真正的交错顺序带到线格式和 JSONL,而 main 依旧塌成一个签名不匹配的区块。所以算法是对的,只是描述里那句话在今天的 Anthropic 链路上无法体现。片段与片段之间、片段与正文之间的顺序会到线上的(见 §3),只有与工具调用的交错不会。

(b) dropDanglingUnsignedTrailingThought 已记录的那个误判,在这条链路上不可达 —— 调用点 #1 的保护作用同样不可达。 上一轮我把它标成「仅代码阅读」,因为它需要 DeepSeek 形态的端点。这次我直接去打它:编排 tool_use → thinking(不发 signature_delta),让未签名片段在流里确实是最后一个。但同样是 deferredToolCalls 的批量吐出,使得工具调用永远是这一轮的最后一个部件,未签名片段因此永远不可能处在尾部 —— 守卫不会触发,PR 与 main 完全一致。OpenAI 兼容转换器采用同样的批量方式(getCompletedToolCalls() 只在 finish_reason 上运行),按代码推断同理,但那条链路我没有实跑。结论:这个已记录的误判在今天两条真实链路上都没有代价,调用点 #1 在那里也相应是空转的。调用点 #2#4(XML 恢复、恢复合并、传输续写)不受影响,依然生效。

5. 变异探针 —— 验证环境不是空转的(图 4)

四个变异,逐个作用在 PR 源码上、重新打包、走同一条真实 CLI 路径,4/4 全部被杀。两个值得单独说:

  • M2(关掉片段拆分条件)精确复现了已记录的限制 2 —— 一个区块带着 SIG-EPISODE-ONE-AAAASIG-EPISODE-TWO-BBBB,对两个区块都无效的签名。这直接证明拆分条件正是让那种形态不可达的承重件,而这正是「限制 2 可以接受」这一论断的基础。
  • M3(把 XML 恢复的判断谓词退回修复前的 .text !== undefined)让推理片段从线格式和 JSONL 中彻底消失 —— 文本和签名一起没了。PR 描述里说的 Model outputs XML-style tool calls as plain text instead of structured function calls in long sessions #8003 交互是真实的。

可合并性

落后 main 56 个提交(上次是 311)。main 自 merge-base 以来没有改过 llm-chat.ts,只有一个提交(#10896)动过 anthropic 目录。对 origin/main@e09a45c5git merge-tree零冲突。当前 head 的 CI 在 Test、Lint & Static、Integration (no-AK)、Desktop Shell、web-shell E2E 上全绿。

reviewDecision 仍是 CHANGES_REQUESTED,被 bot 自己在 2fe2ee32b4(2026-08-12)上的评审钉住,已经落后 6 个提交。这是死结,不是仍然成立的反对意见:26 条未解决线程里,所有 [Critical] 都已标记为 outdated 且被后续提交回应,当前仍然成立的全是 [Suggestion]

遗留事项 —— 与上一轮相同,都不阻塞

  1. 媒体部件现在会写入会话 JSONL。 PR 描述已声明,对 --resume 保真度也是正确取舍。本次仍未覆盖,而且在这条链路上覆盖不了:Anthropic 的 assistant 消息只能承载 text / thinking / tool_use,响应路径从不构造 inlineData 部件。它仍然是 Gemini 链路上需要留意的事。
  2. 措辞小问题(未变): 描述说 recordAssistantTurn「逐字记录所有整合后的部件」。redactStructuredOutputArgsForRecording 返回的是 { functionCall },没有展开原部件,同级字段会被丢掉 —— 实际不可达,所以只是措辞。
  3. 描述里的测试数字过期 —— 369 → 698。
  4. 有两处一行的描述修订会帮到后来的读者:§4(a) 的顺序说法,以及注明限制 3 的误判在今天已发布的链路上不可达(§4(b))。

本次验证未覆盖的部分

真实的 Anthropic API(本次仅用 mock —— §3 指出了唯一值得做一次真机冒烟的请求)、OpenAI Responses 链路(#8169),以及 DeepSeek 供应商路径(仅做了代码阅读)。已记录的限制 2 在 Anthropic 链路上仍不可达,PR 中已正确地为 #8169 标出。

@wenshao

wenshao commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /triage

@wenshao
wenshao enabled auto-merge September 5, 2026 17:13
@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator

Sandboxed verification: ❌ not passed — findings reported (agent verdict) - workflow run

Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check.

Scripted assertions: 57 passed · 0 failed · 57 total

Flakiness gate: ✅ 3 changed test file(s) x 5 identical rounds, no divergence

中文 — 判定:❌ 不通过 · 报告了发现(agent 判定)

沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码(与 base 构建 A/B 对照、无 mock harness 断言、定向门禁)。仅作为评审证据,不构成评审、批准或 CI 检查

脚本断言:57 通过 · 0 失败 · 57 总计

抖动门:✅ 3 changed test file(s) x 5 identical rounds, no divergence

Verification report

<!-- qwen-triage:verify -->
<!-- qwen-triage:verify-substantive -->

Sandboxed verification: ⚠️ findings (agent verdict) - follow-up round 2

Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, an 8-mutant matrix, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check.

Scripted assertions: 57 passed · 0 failed · 57 total

Verdict: findings — every executed assertion passed and the code is verified sound, but this round's delta (main's geminiChat.tsllm-chat.ts rename merged into the branch) left four documentation-level defects in the state that lands, one of which makes the PR's own Reviewer Test Plan report a silent false green. None are code-behavior defects; each is a one-line fix. Verified head: 19dcebfee670d7f698a8f2bcdca2240885d8e023, against merge-base tip e09a45c52f545c4ebb98e4ec6a056d56ca12eaad (HEAD^1 of the merge-ref checkout).

中文摘要
  • 结论findings(非阻塞)。57/57 脚本化断言通过,0 失败;代码行为本身全部验证通过,本轮发现均为文档/描述层面的合并漂移缺陷,其中一处会让 PR 自带的验证命令产生"假绿"。
  • A/B 结论(见「Central claim — A/B cell table」,见证图 01-ab-head-fixed-shapes.png / 02-ab-base-broken-shapes.png):在 main 将 geminiChat.ts 重命名为 llm-chat.ts 之后重新测量,核心修复完好且仍然 load-bearing。base 臂产出合并后的单一 thought 块(只保留第一个签名、截断时把 sig1 错挂在合并文本上、JSONL 丢失媒体部件),head 臂每个推理片段保留各自签名与原始位置;两臂各 16/16(base 臂断言的就是预期的"坏"形状)。
  • 转换器(见「Converter cell table」,03-converter-head.png / 04-converter-base.png):手动模式 thinking 前置修复、相邻 assistant 由"全部 thinking 提前"改为按序拼接、prefill 重排序保住已签名空文本 thinking 块、序列化与位置无关——head 7/7,base 7/7。
  • 变异矩阵05-mutation-matrix-8-of-8-killed.png):8/8 变异体被 PR 自带测试击杀,零存活;新增的组合行 M1b(同时撤掉两个 flush 触发点)red=15,证明分层守卫集合整体 load-bearing。上轮之后新增的测试 accepts a quiet tool-result completion… 经 M1 撤销核心 hunk 后变红(失败信息为行为断言:期望两个 episode、实际收到合并的 reasoning Areasoning B + sigAsigB),非空泛。
  • Findings:F1(Suggestion)Reviewer Test Plan 的命令引用了已不存在的 src/core/geminiChat.test.ts,vitest 将其当作匹配不到文件的过滤器,只跑 114/531 个测试且 exit 0——按该命令验证会得到假绿;F2–F4(nit)重命名合并留下的过期引用:converter.ts:1465{@link ConvertGeminiRequestToAnthropicOptions…} 指向不存在的符号、converter.ts:1743llm-chat.test.ts:16264 仍指向已成 shim 的 geminiChat.ts、PR 正文称 dangling-drop 有"三个调用点"而代码为四个。
  • 未覆盖:S8 场景的 base 臂单元(需 fake timers,改由 M1/M7 击杀覆盖)、逐 commit 归因(depth-2)、全仓门禁、真实端点联线、feat(core): add OpenAI Responses API content generator #8169 转换器。详见 "Not covered"。

Previous-round finding status (re-measured at the new head, not diffed)

The previous round (verified head 6b3e68a, base 7385b27) reported merge-ready, 45/45, with no blocking findings. Neither OID is reachable in this depth-2 checkout, so every carried-forward measurement below was re-run from scratch at head 19dcebf / base e09a45c; no input-closure shortcut was available because main's rename moved the entire code-under-test from geminiChat.ts to llm-chat.ts.

# previous item severity then status at 19dcebf re-measurement
P1 Declared recording change: base drops inlineData/fileData from the JSONL, head records verbatim observation stands S7 cell of this round's A/B: base recorded [{text:'look'}], head recorded [{text:'look'},{inlineData}]; both arms asserted
P2 DeepSeek accepted false positive is real, declared, and pinned by a test observation stands M2 (dangling-drop disabled) turns documents the accepted false positive… red alongside the four protective call sites (red=5)
P3 Correction: plan said 369 tests, actual 465 correction superseded Counts moved again with the rename: llm-chat.test.ts 417, converter.test.ts 114, anthropicContentGenerator.test.ts 168 = 699. The plan's file path is now stale too — that is new finding F1
P4 Methodology note: snapshot baseRefOid drifted; used merge-ref HEAD^1 note stands Same situation: snapshot baseRefOid 60161cb… absent locally; base = HEAD^1 = e09a45c per the CI checkout contract
P5 Mutation matrix 6/6 killed, zero survivors evidence re-measured, extended 8/8 killed at the new head (added combination row M1b and recording mutant M7); witness 05-mutation-matrix-8-of-8-killed.png

Central claim — A/B cell table (re-measured after the rename)

Central claim: turn consolidation in llm-chat.ts (formerly geminiChat.ts) preserves every reasoning episode as its own Part, each with its own thoughtSignature, in its original position relative to tool calls — instead of merging all thought parts into one hoisted blob and keeping only the first signature.

Harness: harness/ab-harness.mjs drives the real LlmChat.sendMessageStream via tsx over the TS source. Zero module mocks on the unit-under-test path; injected collaborators are only a fake ContentGenerator (scripted wire chunks), a fake ChatRecordingService, and a plain config object of real closures. Base arm = scratch worktree at HEAD^1 (e09a45c); witnesses 01-ab-head-fixed-shapes.png, 02-ab-base-broken-shapes.png; raw logs harness/head-run.log, harness/base-run.log.

# scenario BASE cell (broken, asserted) HEAD cell (fixed, asserted)
S1 two episodes interleaved with two tool calls [{text:"AB",thought,sigA}, call1, call2] — merged, hoisted, sigB lost in history and JSONL [{A,sigA}, call1, {B,sigB}, call2] — both signatures, original positions
S2 one episode, signature split across two chunks thoughtSignature:"s1"truncated thoughtSignature:"s1s2" — concatenated
S3 back-to-back episodes, no intervening tool call [{text:"AB",thought,sigA}, {final}] [{A,sigA}, {B,sigB}, {final}]
S4 MAX_TOKENS truncation mid-episode-2 after a tool call [{text:"ep1ep2 partial",thought,sig1}, call1]sig1 (valid for ep1 only) bound to merged text [{ep1,sig1}, call1] — dangling unsigned trailing episode dropped
S5 reasoning episode + XML tool-call recovery, same turn episode survives (parity) episode survives in place with text+signature (parity)
S6 plain-text part with stray thoughtSignature + XML (leak shape) raw XML consumed, no leak byte-identical to base — parity cell; the leak existed mid-PR-history and is closed in the final state
S7 declared change: media part in a model turn history carries inlineData but recorded JSONL drops it ([{text:'look'}]) recorded JSONL carries inlineData verbatim
S8 NEW since last round: quiet tool-result completion, two signed episodes not run in this harness (needs fake timers + retry exhaustion) — see Not covered covered by mutants M1/M7 instead

Counts: head 16/16, base 16/16. Every flip cell (S1–S4, S7-recorded) proves the change load-bearing in the renamed file; S5/S6 are parity cells. The base arm producing the broken shapes is also the contamination control: the base worktree resolved external deps by walking up to the root node_modules plus a symlinked packages/core/node_modules (verified to contain no @qwen-code/*), and packages/core/src imports @qwen-code/* only in providers/__tests__/, outside this closure — so a contaminated control would have emitted head shapes, and it did not.

Converter cell table (secondary claim, re-measured)

Harness: harness/converter-harness.mjs drives the real AnthropicContentConverter.convertLlmRequestToAnthropic and asserts the exact emitted Anthropic block arrays. Witnesses 03-converter-head.png, 04-converter-base.png; logs harness/converter-{head,base}.log.

# scenario BASE HEAD
C1 manual mode, text-leading tool_use turn ships the invalid [text, thinking, tool_use] as-is repaired to [thinking, text, tool_use]
C2 adaptive mode, same turn chronological pass-through chronological pass-through (parity)
C3a adjacent assistant merge, adaptive hoist: [thinkingX, thinkingY, textA, tool_use] concat: [textA, thinkingX, thinkingY, tool_use]
C3b same merge, manual mode hoisted concat then first thinking run to front — same result here
C4 same turn serialized as latest vs as prior (head invariant) trivially identical (mechanism absent) byte-identical — position-independent, prompt-cache prefix stable
C5 whitespace-only trailing assistant (prefill artifact) popped; earlier tool turn carries signed empty-text thinking pop-after-strip ordering → promoted turn loses its signed thinking('')[tool_use] only pop-first ordering → signed empty-text thinking kept: [thinking('',sigEmpty), tool_use]
C6 two thinking runs, manual mode pass-through only the FIRST run moves: [thinkingF, text, tu1, thinkingS, tu2]

Counts: head 7/7, base 7/7.

Mutation matrix (vacuity of the PR's tests at the new head)

Driver: harness/matrix-driver.mjs — asserts each mutant's anchor occurs exactly once, applies it to the head tree, runs the targeted suite, reads red titles from the JUNIT report, restores via git checkout -- and verifies the file is clean. Witness 05-mutation-matrix-8-of-8-killed.png; log harness/matrix-run.log.

mutant guard removed/reverted suite killed red key test red
M1 episode-split boundary condition deleted llm-chat.test.ts 2 back-to-back split + the new quiet-signed test
M1b COMBINATION: both flush triggers removed llm-chat.test.ts 15 interleaved-episode test (proves the layered set is load-bearing)
M2 dropDanglingUnsignedTrailingThought disabled llm-chat.test.ts 5 truncation test + accepted-false-positive pin
M3 removal-loop predicate → bare .text !== undefined llm-chat.test.ts 4 episode-preservation-under-XML-recovery test
M6 removal-loop predicate → stricter isValidNonThoughtTextPart llm-chat.test.ts 2 stray-signature XML-leak test
M7 JSONL recording drops thought parts llm-chat.test.ts 3 JSONL-recording test + quiet-signed test
M4 leading-thinking scoped to latest message only converter.test.ts 2 every-tool_use-turn + position-independence tests
M5 merge restored to hoist-all-thinking converter.test.ts 3 chronological-merge + multi-run tests

Zero survivors. Notes:

  • Vacuity of the delta test (the one commit new since last round, 14292a73 "cover quiet signed reasoning persistence"): M1 turns accepts a quiet tool-result completion with every signed reasoning episode in history and JSONL red with a behavioral assertion, not a crash: AssertionError: expected { role: 'model', parts: [ { …(3) } ] } to deeply equal … with the diff showing expected reasoning A/sigA + reasoning B/sigB versus received merged reasoning Areasoning B with thoughtSignature: "sigAsigB". The new test is load-bearing.
  • Layered guards adjudicated: M1 alone kills only the back-to-back and quiet-signed shapes because the interleaved shape is held by the other flush trigger (flush on non-thought part). The M1b combination row (red=15) is the proof the two-guard set is load-bearing, and reclassifies M1's narrow kill as expected, not as a survivor.
  • Same-file positive control: every mutant lands in the very file its killing suite imports (llm-chat.tsllm-chat.test.ts, converter.tsconverter.test.ts), and the unmutated gate run (417 passed) is the green control. The first matrix run reported 8/8 survived because the driver parsed the console reporter's glyphs; switching the oracle to the JUNIT report fixed the harness, not the code — recorded here so the discarded first log is not misread as evidence of vacuity.

Targeted gates

Witness 06-gates-and-stale-plan-command.png; logs harness/gate-{llm-chat,converter,acg}.log.

gate result
src/core/llm-chat.test.ts 417 passed, 0 failed
src/core/anthropicContentGenerator/converter.test.ts 114 passed, 0 failed
src/core/anthropicContentGenerator/anthropicContentGenerator.test.ts 168 passed, 0 failed

Suite liveness is proven by the matrix: eight distinct mutants turned these suites red.

Corrections (to the description, not requests to change code)

  • The PR body and several code comments describe the changed file as geminiChat.ts and the class as GeminiChat. At this head, main has renamed both: the code under test is packages/core/src/core/llm-chat.ts / class LlmChat, and geminiChat.ts is a 205-byte deprecated re-export shim. The behaviour described is intact (A/B above); only the names in the description are stale.
  • The body states dropDanglingUnsignedTrailingThought is "applied at three call sites". The merged code applies it at four (the fourth: immediately before a transport-continuation prefix is inserted into a thought-only parts array), and the function's own doc comment says four. The body's enumeration predates that call site.

Findings (ordered by severity; none are code-behavior defects)

F1 — Suggestion: the Reviewer Test Plan command silently skips the primary fix's suite and exits 0

The plan's verification command is:

cd packages/core && npx vitest run src/core/geminiChat.test.ts src/core/anthropicContentGenerator/converter.test.ts

src/core/geminiChat.test.ts no longer exists (main renamed it to llm-chat.test.ts). Vitest treats a path filter that matches no file as "nothing to run" rather than an error, so the command ran only converter.test.ts (114 tests) and exited 0 — silently skipping llm-chat.test.ts's 417 tests, which is where the entire primary fix and all 27 of the PR's new llm-chat test cases live. Measured verbatim at head; witness 06-gates-and-stale-plan-command.png.

Blast radius: any human reviewer or local automation following the plan gets a green result covering 114 of the 531 relevant tests. Repo CI is unaffected (it runs the full suite). The fix is one line: replace src/core/geminiChat.test.ts with src/core/llm-chat.test.ts. This is the sharpest of the four because it fails quietly — the same ordering principle as the round's other findings, where a wrong answer nobody is told about outranks a loud failure.

F2 — nit: broken TSDoc link to a symbol that does not exist

packages/core/src/core/anthropicContentGenerator/converter.ts:1465 reads {@link ConvertGeminiRequestToAnthropicOptions.ensureLeadingAssistantThinking}. The interface is ConvertLlmRequestToAnthropicOptions (converter.ts:97); ConvertGeminiRequestToAnthropicOptions exists nowhere in packages/core/src or packages/cli/src and has no alias export (unlike the class, which main kept as export { LlmChat as GeminiChat }). The link was written against the pre-rename name and the merge did not update it.

F3 — nit: two stale pointers at geminiChat.ts, now a shim

converter.ts:1743 ("see geminiChat.ts's flushThoughtEpisode") and llm-chat.test.ts:16264 ("geminiChat.ts's recovery loop skips recovery only when") both point at a file that is now a 205-byte export * from './llm-chat.js' shim; flushThoughtEpisode and the recovery loop live in llm-chat.ts. Both lines are PR-added (git diff HEAD^1..HEAD shows them as +), so the drift entered with this PR's merge of main.

F4 — nit: body's call-site count is one behind the code

See Corrections: body says three call sites for dropDanglingUnsignedTrailingThought, code has four.

No injection-style instructions were present in the PR text.

Not covered

  • S8's base-arm cell in the tsx A/B harness: the quiet tool-result completion path requires the multi-attempt retry loop (acceptQuietToolResultCompletion is true only on the attempt after a first attempt throws NO_TOOL_RESULT_PROGRESS) and the PR's own test drives it with vi.useFakeTimers() plus a 35 s advance, which a plain-tsx harness cannot reproduce. Its behaviour is instead pinned by mutants M1 and M7 in the head tree. The head-side shape is asserted by the PR's own test, which the gates ran green.
  • Per-commit attribution: depth-2 checkout; git rev-list HEAD^1..HEAD^2 yields 1 locally while the snapshot lists 20 commits, so only the aggregate HEAD^1..HEAD diff was verified and no per-commit table is presented.
  • Repo-wide suite, lint, typecheck: not re-run; the PR's own CI covers them. Only the three affected test files were executed here.
  • Live-wire validation: no Anthropic/OpenAI credentials in this sandbox; the converter oracle is the emitted block shape, not API acceptance.
  • OpenAI Responses wire (feat(core): add OpenAI Responses API content generator #8169): not present at this base; the Responses-shaped episode claim is covered only by the suite's Gemini-Part-shaped fixture.
  • Performance/ladder probes: not applicable — the change is a linear single-pass walk with string concatenation and adds no regex/scanner over untrusted text.
  • The first matrix run (8/8 apparent survivors) is discarded harness output, not evidence; see the matrix notes.

Methodology

CI verify container (node:22-bookworm, node v22.23.2), merge-ref checkout at depth 2; npm ci + npm run build completed before this round. The A/B and converter harnesses import the real production TS modules with tsx — no module mocking anywhere in the unit-under-test path; collaborators enter only through constructor/config seams. The base control ran in a scratch worktree at HEAD^1 resolving external deps through the root node_modules (the PR touches no package.json/lockfile) plus a symlinked packages/core/node_modules verified to contain no @qwen-code/*; the code-under-test closure was grep-verified to import no @qwen-code/*, and the base arm's emission of the broken shapes is the behavioural confirmation that no head code leaked into the control. Mutations ran directly in the head tree with per-mutant anchor-uniqueness assertions and git checkout -- restore verified clean by git status; red titles were read from the JUNIT report vitest writes each run. The base worktree was removed after the A/B cells were captured. Raw per-arm logs, all harness scripts, and the six evidence images live in tmp/pr8260-verify-20260905-172727/ (harness/, evidence/).

Flakiness gate log

rounds=5 files=3 skipped=0
file packages/core/src/core/anthropicContentGenerator/anthropicContentGenerator.test.ts: (cd packages/core) npx --no-install vitest run ./src/core/anthropicContentGenerator/anthropicContentGenerator.test.ts
file packages/core/src/core/anthropicContentGenerator/converter.test.ts: (cd packages/core) npx --no-install vitest run ./src/core/anthropicContentGenerator/converter.test.ts
file packages/core/src/core/llm-chat.test.ts: (cd packages/core) npx --no-install vitest run ./src/core/llm-chat.test.ts


per-file results (P=pass F=fail I=infra-exit, one letter per run):
  packages/core/src/core/anthropicContentGenerator/anthropicContentGenerator.test.ts: PPPPP
  packages/core/src/core/anthropicContentGenerator/converter.test.ts: PPPPP
  packages/core/src/core/llm-chat.test.ts: PPPPP

verdict: pass
summary: 3 changed test file(s) x 5 identical rounds, no divergence

--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/core/src/core/anthropicContentGenerator/anthropicContentGenerator.test.ts: P (exit 0)
round 1 · packages/core/src/core/anthropicContentGenerator/converter.test.ts: P (exit 0)
round 1 · packages/core/src/core/llm-chat.test.ts: P (exit 0)
round 2 · packages/core/src/core/anthropicContentGenerator/anthropicContentGenerator.test.ts: P (exit 0)
round 2 · packages/core/src/core/anthropicContentGenerator/converter.test.ts: P (exit 0)
round 2 · packages/core/src/core/llm-chat.test.ts: P (exit 0)
round 3 · packages/core/src/core/anthropicContentGenerator/anthropicContentGenerator.test.ts: P (exit 0)
round 3 · packages/core/src/core/anthropicContentGenerator/converter.test.ts: P (exit 0)
round 3 · packages/core/src/core/llm-chat.test.ts: P (exit 0)
round 4 · packages/core/src/core/anthropicContentGenerator/anthropicContentGenerator.test.ts: P (exit 0)
round 4 · packages/core/src/core/anthropicContentGenerator/converter.test.ts: P (exit 0)
round 4 · packages/core/src/core/llm-chat.test.ts: P (exit 0)
round 5 · packages/core/src/core/anthropicContentGenerator/anthropicContentGenerator.test.ts: P (exit 0)
round 5 · packages/core/src/core/anthropicContentGenerator/converter.test.ts: P (exit 0)
round 5 · packages/core/src/core/llm-chat.test.ts: P (exit 0)

Evidence images

01-ab-head-fixed-shapes

02-ab-base-broken-shapes

03-converter-head

04-converter-base

05-mutation-matrix-8-of-8-killed

06-gates-and-stale-plan-command

Harness scripts and raw logs are in the workflow run artifacts (7-day retention).

Qwen Code · sandboxed verification

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review/self-reported The linked issue was opened by the PR author (self-reported)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

geminiChat.ts history consolidation keeps only the first thoughtSignature per turn, dropping later reasoning episodes

6 participants